On Wed, Sep 27, 2023 at 6:54 PM Christopher Schultz <ch...@christopherschultz.net> wrote: > > Rémy, > > This is a good one. Nice catch.
About UpgradeInfo itself, I didn't consider it was reasonable to have concurrent IO operations since that would fail, so I didn't add sync or adders (the stats remain regular volatile without sync, which was flagged by coverity). Rémy > > -chris > > On 9/27/23 09:59, r...@apache.org wrote: > > This is an automated email from the ASF dual-hosted git repository. > > > > remm pushed a commit to branch main > > in repository https://gitbox.apache.org/repos/asf/tomcat.git > > > > > > The following commit(s) were added to refs/heads/main by this push: > > new 28e1010655 Refactor to remove syncs for upgraded connections > > statistics > > 28e1010655 is described below > > > > commit 28e1010655d2952a94550fa1156eed5f280f2620 > > Author: remm <r...@apache.org> > > AuthorDate: Wed Sep 27 15:59:17 2023 +0200 > > > > Refactor to remove syncs for upgraded connections statistics > > > > Found with coverity, while looking at UpgradeInfo. > > --- > > .../coyote/http11/upgrade/UpgradeGroupInfo.java | 63 > > ++++++++++++---------- > > webapps/docs/changelog.xml | 4 ++ > > 2 files changed, 38 insertions(+), 29 deletions(-) > > > > diff --git a/java/org/apache/coyote/http11/upgrade/UpgradeGroupInfo.java > > b/java/org/apache/coyote/http11/upgrade/UpgradeGroupInfo.java > > index 7d729767d8..dc44062461 100644 > > --- a/java/org/apache/coyote/http11/upgrade/UpgradeGroupInfo.java > > +++ b/java/org/apache/coyote/http11/upgrade/UpgradeGroupInfo.java > > @@ -16,8 +16,9 @@ > > */ > > package org.apache.coyote.http11.upgrade; > > > > -import java.util.ArrayList; > > -import java.util.List; > > +import java.util.Set; > > +import java.util.concurrent.ConcurrentHashMap; > > +import java.util.concurrent.atomic.LongAdder; > > > > import org.apache.tomcat.util.modeler.BaseModelMBean; > > > > @@ -26,85 +27,89 @@ import org.apache.tomcat.util.modeler.BaseModelMBean; > > */ > > public class UpgradeGroupInfo extends BaseModelMBean { > > > > - private final List<UpgradeInfo> upgradeInfos = new ArrayList<>(); > > + private final Set<UpgradeInfo> upgradeInfos = (new > > ConcurrentHashMap<UpgradeInfo,Boolean>()).keySet(Boolean.TRUE); > > > > - private long deadBytesReceived = 0; > > - private long deadBytesSent = 0; > > - private long deadMsgsReceived = 0; > > - private long deadMsgsSent = 0; > > + private LongAdder deadBytesReceived = new LongAdder(); > > + private LongAdder deadBytesSent = new LongAdder(); > > + private LongAdder deadMsgsReceived = new LongAdder(); > > + private LongAdder deadMsgsSent = new LongAdder(); > > > > > > - public synchronized void addUpgradeInfo(UpgradeInfo ui) { > > + public void addUpgradeInfo(UpgradeInfo ui) { > > upgradeInfos.add(ui); > > } > > > > > > - public synchronized void removeUpgradeInfo(UpgradeInfo ui) { > > + public void removeUpgradeInfo(UpgradeInfo ui) { > > if (ui != null) { > > - deadBytesReceived += ui.getBytesReceived(); > > - deadBytesSent += ui.getBytesSent(); > > - deadMsgsReceived += ui.getMsgsReceived(); > > - deadMsgsSent += ui.getMsgsSent(); > > + deadBytesReceived.add(ui.getBytesReceived()); > > + deadBytesSent.add(ui.getBytesSent()); > > + deadMsgsReceived.add(ui.getMsgsReceived()); > > + deadMsgsSent.add(ui.getMsgsSent()); > > > > upgradeInfos.remove(ui); > > } > > } > > > > > > - public synchronized long getBytesReceived() { > > - long bytes = deadBytesReceived; > > + public long getBytesReceived() { > > + long bytes = deadBytesReceived.longValue(); > > for (UpgradeInfo ui : upgradeInfos) { > > bytes += ui.getBytesReceived(); > > } > > return bytes; > > } > > - public synchronized void setBytesReceived(long bytesReceived) { > > - deadBytesReceived = bytesReceived; > > + public void setBytesReceived(long bytesReceived) { > > + deadBytesReceived.reset(); > > + deadBytesReceived.add(bytesReceived); > > for (UpgradeInfo ui : upgradeInfos) { > > ui.setBytesReceived(bytesReceived); > > } > > } > > > > > > - public synchronized long getBytesSent() { > > - long bytes = deadBytesSent; > > + public long getBytesSent() { > > + long bytes = deadBytesSent.longValue(); > > for (UpgradeInfo ui : upgradeInfos) { > > bytes += ui.getBytesSent(); > > } > > return bytes; > > } > > - public synchronized void setBytesSent(long bytesSent) { > > - deadBytesSent = bytesSent; > > + public void setBytesSent(long bytesSent) { > > + deadBytesSent.reset(); > > + deadBytesSent.add(bytesSent); > > for (UpgradeInfo ui : upgradeInfos) { > > ui.setBytesSent(bytesSent); > > } > > } > > > > > > - public synchronized long getMsgsReceived() { > > - long msgs = deadMsgsReceived; > > + public long getMsgsReceived() { > > + long msgs = deadMsgsReceived.longValue(); > > for (UpgradeInfo ui : upgradeInfos) { > > msgs += ui.getMsgsReceived(); > > } > > return msgs; > > } > > - public synchronized void setMsgsReceived(long msgsReceived) { > > - deadMsgsReceived = msgsReceived; > > + public void setMsgsReceived(long msgsReceived) { > > + deadMsgsReceived.reset(); > > + deadMsgsReceived.add(msgsReceived); > > for (UpgradeInfo ui : upgradeInfos) { > > ui.setMsgsReceived(msgsReceived); > > } > > } > > > > > > - public synchronized long getMsgsSent() { > > - long msgs = deadMsgsSent; > > + public long getMsgsSent() { > > + long msgs = deadMsgsSent.longValue(); > > for (UpgradeInfo ui : upgradeInfos) { > > msgs += ui.getMsgsSent(); > > } > > return msgs; > > } > > - public synchronized void setMsgsSent(long msgsSent) { > > - deadMsgsSent = msgsSent; > > + public void setMsgsSent(long msgsSent) { > > + deadMsgsSent.reset(); > > + deadMsgsSent.add(msgsSent); > > for (UpgradeInfo ui : upgradeInfos) { > > ui.setMsgsSent(msgsSent); > > } > > diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml > > index aa98ce6c99..424f2bfb1d 100644 > > --- a/webapps/docs/changelog.xml > > +++ b/webapps/docs/changelog.xml > > @@ -156,6 +156,10 @@ > > <fix> > > Avoid rare thread safety issue accessing message digest map. > > (remm) > > </fix> > > + <fix> > > + Improve statistics collection for upgraded connections under load. > > + (remm) > > + </fix> > > </changelog> > > </subsection> > > <subsection name="Jasper"> > > > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > > For additional commands, e-mail: dev-h...@tomcat.apache.org > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
