You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by re...@apache.org on 2023/09/27 13:59:35 UTC
[tomcat] branch main updated: Refactor to remove syncs for upgraded connections statistics
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 <re...@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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch main updated: Refactor to remove syncs for upgraded connections statistics
Posted by Rémy Maucherat <re...@apache.org>.
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, remm@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 <re...@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-unsubscribe@tomcat.apache.org
> > For additional commands, e-mail: dev-help@tomcat.apache.org
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch main updated: Refactor to remove syncs for upgraded connections statistics
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Rémy,
This is a good one. Nice catch.
-chris
On 9/27/23 09:59, remm@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 <re...@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-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org