Investigate removal of locks in java.lang.Thread

[babsingh]

Issue

High lock contention can due to the below locks in j.l.Thread class.

JDK8, 11, 17 (Thread.java from the OpenJ9 repo is used)
private static final class ThreadLock {}
private Object lock = new ThreadLock();

JDK21+ (Thread.java from the extension repo is used)
final Object interruptLock = new Object();

• In JDK8/11/17, there are ~15 synchronized blocks associated to the above lock.
• In JDK21+, there are ~13 synchronized blocks associated to the lock.

By relaxing locking in some j.l.Thread APIs, 10% perf improvement was observed in a Netty application run through the Quarkus framework.

Goal

There is an opportunity to improve the perf of OpenJ9's j.l.Thread APIs by either removing the lock in some cases or utilizing separate locks for some critical sections. This issue has been opened to initiate a discussion on the best approach to improve perf by not hindering functionality.

Existing case (see https://github.com/eclipse-openj9/openj9/issues/17251): We are improving Thread.getState by removing the need of a lock and complex work to evaluate the state.

Potential new cases:

• Remove the lock in places where we just read the fields (e.g. isAlive, isDead).
• The global lock should only be used in start, join and other key methods. On the other hand, the interruptLock should only be used for interrupt specific work.
• Some methods don't need to be thread-safe, but the above global lock is used (e.g. setPriority, setDaemon).

[github-actions[bot]]

Issue Number: 20414 Status: Open Recommended Components: comp:vm, comp:gc, comp:jclextensions Recommended Assignees: jasonfengj9, babsingh, gacholio

[ThanHenderson]

A few of these synchronized(interruptLock) were quite recently added, mostly to resolve crashes/race conditions. (e.g. PR 1 and PR 2). Those PRs use the lock in, I think, appropriate ways; locking w.r.t. the thread interrupt implementation.

However, PR 1 states that the crashes occurred due to unexpected/inconsistent state of the eetop/threadRef value. In our Thread.java extension repo code, we are protecting most (though not all) eetop reads with the synchronized(interruptLock), do we need to be using the interruptLock to guard eetop access?

In JDK21+, there are ~13 synchronized blocks associated to the lock.

This value may be over estimating the problem slightly since synchronized blocks are reentrant and some of these paths overlap. (e.g. isDead() is only ever called from getState()).

By relaxing locking in some j.l.Thread APIs, 10% perf improvement was observed in a Netty application run through the Quarkus framework.

Which locks were removed for this testing?

[babsingh]

In JDK17, contention is seen in the following code path:

[]-----------------  24  1      0   0.00  48.76           0       29728    io/vertx/core/http/impl/AssembledHttpResponse.release()Z
 +[]---------------  25  1      0   0.00  48.76           0       29728    io/netty/buffer/AbstractReferenceCountedByteBuf.release()Z
   +[]-------------  26  1      0   0.00  48.76           0       29728    io/netty/buffer/AbstractReferenceCountedByteBuf.handleRelease(Z)Z
     +[]-----------  27  1      0   0.00  48.76           0       29728    io/netty/buffer/PooledByteBuf.deallocate()V
       +[]---------  28  1      0   0.00  48.76           0       29728    io/netty/util/Recycler$DefaultHandle.unguardedRecycle(Ljava/lang/Object;)V
         +[]-------  29  1      0   0.00  48.76           0       29728    io/netty/util/Recycler$LocalPool.release(Lio/netty/util/Recycler$DefaultHandle;Z)V
           +[]-----  30  1      0   0.00  48.76           0       29728    io/netty/util/Recycler$LocalPool.isTerminated(Ljava/lang/Thread;)Z
             +[]---  31  1      0   0.00  48.76           0       29728    java/lang/Thread.isAlive()Z
               +[]-  32  1      0  48.76  48.76       29728       29728    (java/lang/Thread$ThreadLock)

This contention should be fixed by https://github.com/eclipse-openj9/openj9/pull/20415.

[babsingh]

In JDK21, contention is seen in a different code path:

[]-------------------   0  1      1   0.00  33.41           0     2009748    0000dd0b_executor-thread-1_pidtid
 +[]-----------------   1  1      0   0.00  33.41           0     2009748    java/lang/Thread.run()V
   +[]---------------   2  1      0   0.00  33.41           0     2009748    java/lang/Thread.runWith(Ljava/lang/Object;Ljava/lang/Runnable;)V
     +[]-------------   3  1      0   0.00  33.41           0     2009748    io/netty/util/concurrent/FastThreadLocalRunnable.run()V
       +[]-----------   4  1      0   0.00  33.41           0     2009748    org/jboss/threads/ThreadLocalResettingRunnable.run()V
         +[]---------   5  1      0   0.00  33.41           0     2009748    org/jboss/threads/DelegatingRunnable.run()V
           +[]-------   6  1      0   0.00  33.41           0     2009748    org/jboss/threads/EnhancedQueueExecutor$ThreadBody.run()V
             +[]-----   7  1      0   0.00  33.41           0     2009748    java/lang/Thread.interrupted()Z
               +[]---   8  1      0   0.00  33.41           0     2009748    java/lang/Thread.getAndClearInterrupt()Z
                 +[]-   9  1      0  33.41  33.41     2009748     2009748    (java/lang/Object)

A synchronized block was added in getAndClearInterrupt to fix https://github.com/eclipse-openj9/openj9/issues/15465. We might not be able to remove synchronization, but we can relax it in the below manner:

    boolean getAndClearInterrupt() {
        boolean oldValue = interrupted;
        if (oldValue) {
            synchronized(interruptLock) { <--- lock will only be acquired in the true case
                oldValue = interrupted;
                if (oldValue) {
                    interrupted = false;
                    clearInterruptEvent();
                }
            }
        }
        return oldValue;
    }