8336707: Contention of ForkJoinPool grows when stealing works #21507
Conversation
|
👋 Welcome back dl! A progress list of the required criteria for merging this PR into |
|
@DougLea This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 37 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
DougLea
changed the title
openjdk
bot
changed the title
src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java
Outdated
Show resolved
Hide resolved
| ForkJoinPool p = common = (System.getSecurityManager() == null) ? | ||
| new ForkJoinPool((byte)0) : | ||
| AccessController.doPrivileged(new PrivilegedAction<>() { | ||
| public ForkJoinPool run() { | ||
| return new ForkJoinPool((byte)0); }}); |
There was a problem hiding this comment.
@AlanBateman Any recommendation as to what is ideal here with SM removed? /cc @DougLea
There was a problem hiding this comment.
I moved this to end so that in next pass with SM removed, the static init will end with just:
common = new ForkJoinPool((byte)0).
(Which will be sure to be done after all the other static init finiahes.)
There was a problem hiding this comment.
@DougLea I must've forgotten about addressing SM separately. 👍
| /** | ||
| * Clears ThreadLocals, and if necessary resets ContextClassLoader | ||
| */ | ||
| final void resetThreadLocals() { | ||
| if (U.getReference(this, THREADLOCALS) != null) | ||
| U.putReference(this, THREADLOCALS, null); | ||
| if (U.getReference(this, INHERITABLETHREADLOCALS) != null) | ||
| U.putReference(this, INHERITABLETHREADLOCALS, null); | ||
| if ((this instanceof InnocuousForkJoinWorkerThread) && | ||
| ((InnocuousForkJoinWorkerThread)this).needCCLReset()) | ||
| super.setContextClassLoader(ClassLoader.getSystemClassLoader()); | ||
| } | ||
|
|
||
| private static final Unsafe U = Unsafe.getUnsafe(); | ||
| private static final long THREADLOCALS | ||
| = U.objectFieldOffset(Thread.class, "threadLocals"); | ||
| private static final long INHERITABLETHREADLOCALS | ||
| = U.objectFieldOffset(Thread.class, "inheritableThreadLocals"); |
There was a problem hiding this comment.
@AlanBateman Thoughts here? 🤔
There was a problem hiding this comment.
resetThreadLocals looks good although. A discussion point is whether reset should be done for all FJP instances, not just the common pool but not this PR.
There was a problem hiding this comment.
Thanks Alan. Yes, I think it should be considered to do it for all FJP instances.
There was a problem hiding this comment.
Well, almost all. There is a protected FJWT constructor with explicit arg about preserving ThreadLocals,, in which case it should also not reset CCL. (Also, I just noticed that the Thread ctor with !inherit now also forces using System classLoader, so the FJWT ctor no longer needs to in this case.) Will do as part of SM-removal pr coming next.
| public void setContextClassLoader(ClassLoader cl) { | ||
| if (cl != null && ClassLoader.getSystemClassLoader() != cl) | ||
| if (System.getSecurityManager() != null && |
There was a problem hiding this comment.
@AlanBateman Alternatives for this? 🤔
There was a problem hiding this comment.
I don't think it needs an alternative, instead L270-272 will be removed as part of the SM cleanup.
There was a problem hiding this comment.
Sounds like a good plan 👍
|
/integrate |
|
Going to push as commit 18df6fd.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
This addresses tendencies in previous update to increase fencing, scanning, and signalling that can increase contention, and slow down performance especially on ARM platforms. It also uses more ARM-friendly constructions to reduce overhead (leading to several changes that all of the same form),
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21507/head:pull/21507$ git checkout pull/21507Update a local copy of the PR:
$ git checkout pull/21507$ git pull https://git.openjdk.org/jdk.git pull/21507/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 21507View PR using the GUI difftool:
$ git pr show -t 21507Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21507.diff
Using Webrev
Link to Webrev Comment