[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4098: ARTEMIS-3848 High CPU usage on ReadWriteLocks

[GitBox <gi...@apache.org>]

gemmellr commented on code in PR #4098:
URL: https://github.com/apache/activemq-artemis/pull/4098#discussion_r881691801


##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyConnection.java:
##########
@@ -62,7 +61,7 @@ public class NettyConnection implements Connection {
     * here for when the connection (or Netty Channel) becomes available again.
     */
    private final List<ReadyListener> readyListeners = new ArrayList<>();
-   private final FastThreadLocal<ArrayList<ReadyListener>> localListenersPool = new FastThreadLocal<>();
+   private static final ThreadLocal<ArrayList<ReadyListener>> readyListenersPool = new ThreadLocal<>();

Review Comment:
   Seems like it was unrelated, thats about Netty threads using an optimised form of ThreadLocal, and I guess expectation was it would be a Netty thread calling.



##########
pom.xml:
##########
@@ -1131,7 +1131,7 @@
                               <arg>--add-exports=jdk.unsupported/sun.misc=ALL-UNNAMED</arg>
                               <arg>--add-exports=java.base/jdk.internal.misc=ALL-UNNAMED</arg>
                               <arg>-XDcompilePolicy=simple</arg>
-                              <arg>-Xplugin:ErrorProne -Xep:MissingOverride:ERROR -Xep:NonAtomicVolatileUpdate:ERROR -Xep:SynchronizeOnNonFinalField:ERROR -Xep:StaticQualifiedUsingExpression:ERROR -Xep:WaitNotInLoop:ERROR -XepExcludedPaths:.*/generated-sources/.*</arg>
+                              <arg>-Xplugin:ErrorProne -Xep:ThreadLocalUsage:ERROR -Xep:MissingOverride:ERROR -Xep:NonAtomicVolatileUpdate:ERROR -Xep:SynchronizeOnNonFinalField:ERROR -Xep:StaticQualifiedUsingExpression:ERROR -Xep:WaitNotInLoop:ERROR -XepExcludedPaths:.*/generated-sources/.*</arg>

Review Comment:
   You missed updating the similar-bug-different config in the other profile below



##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/nullpm/NullStorageManager.java:
##########
@@ -653,7 +653,7 @@ public boolean addToPage(PagingStore store,
        * The reasoning is that exposing the lock is more explicit and therefore `less bad`.
        */
       if (store != null) {
-         return store.page(msg, tx, listCtx, null);
+         return store.page(msg, tx, listCtx);

Review Comment:
   Needs adjusted similar to other comment (arguably made no sense to begin with as it didnt pass a lock..)



##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/AbstractJournalStorageManager.java:
##########
@@ -2160,7 +2160,9 @@ public boolean addToPage(PagingStore store, Message msg, Transaction tx, RouteCo
        * <p>
        * The reasoning is that exposing the lock is more explicit and therefore `less bad`.
        */
-      return store.page(msg, tx, listCtx, storageManagerLock.readLock());
+      try (ArtemisCloseable closeable = closeableReadLock()) {
+         return store.page(msg, tx, listCtx);
+      }

Review Comment:
   The large comment above seems like it needs adjusting/removed as the lock is no longer passed/exposed.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org