-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reduce lock contention in Thread.getAndClearInterrupt #215
Conversation
if (oldValue) { | ||
interrupted = false; | ||
clearInterruptEvent(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the equivalent VirtualThread.java
method, the synchronized
block is just within the if-statement:
openj9-openjdk-jdk21/src/java.base/share/classes/java/lang/VirtualThread.java
Lines 885 to 895 in cd40810
boolean getAndClearInterrupt() { | |
assert Thread.currentThread() == this; | |
boolean oldValue = interrupted; | |
if (oldValue) { | |
synchronized (interruptLock) { | |
interrupted = false; | |
carrierThread.clearInterrupt(); | |
} | |
} | |
return oldValue; | |
} |
Is there a reason as to why that approach wouldn't work here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionally, no difference will be seen. In the case, where there is high contention i.e. multiple threads attempt to enter the synchronized
block and clear the interrupt, only one thread will invoke the native method (Thread.interruptedImpl
) due to the extra if
statement. Otherwise, all threads that enter the synchronized
block will invoke the native method; and this will lower the throughput when resources are scarce. At the cost of an extra if
statement, we lower the invocations of the native method in the worst case to improve perf.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is such a performance benefit, we should likely apply this to the VirtualThread.java
method too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting for perf data. Will apply the change to VirtualThread.java
after reviewing perf data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Related lock contention is completely removed, and there is a 15% perf improvement.
- Also, VirtualThread.java has been updated.
Lock contention in Thread.getAndClearInterrupt is reduced by acquiring the lock only when the value of the Thread.interrupted field is true. Related: eclipse-openj9/openj9#20414 Signed-off-by: Babneet Singh <[email protected]>
jenkins test sanity alinux64 jdk21 |
jenkins test extended.system zlinux jdk21 |
jenkins test extended.functional plinux jdk21 |
Lock contention in
Thread.getAndClearInterrupt
is reduced byacquiring the lock only when the value of the
Thread.interrupted
field is
true
.Related: eclipse-openj9/openj9#20414