Re: RFR: 8322732: ForkJoinPool may underutilize cores in async mode [v7]
On Wed, 29 May 2024 14:09:51 GMT, Viktor Klang wrote: >> Doug Lea has updated the pull request with a new target base due to a merge >> or a rebase. The incremental webrev excludes the unrelated changes brought >> in by the merge/rebase. The pull request contains 41 additional commits >> since the last revision: >> >> - Merge branch 'openjdk:master' into JDK-8322732 >> - Add test for utilization with interdependent tasks >> - Un-misplace onSpinWait call >> - Adjust control flow >> - Reduce memory stalls >> - Merge branch 'openjdk:master' into JDK-8322732 >> - More performance tradoffs >> - Address review comments >> - Merge branch 'openjdk:master' into JDK-8322732 >> - Repack some fields; adjust control flow >> - ... and 31 more: https://git.openjdk.org/jdk/compare/ae4752d2...cf5fe55c > > src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 1969: > >> 1967: else if ((e & SHUTDOWN) == 0) >> 1968: return 0; >> 1969: else if (compareAndSetCtl(c, c) && casRunState(e, e | >> STOP)) > > is the `compareAndSetCtl(c, c)` needed for the read+write fence if ctl == c? yes. - PR Review Comment: https://git.openjdk.org/jdk/pull/19131#discussion_r1619209374
Re: RFR: 8322732: ForkJoinPool may underutilize cores in async mode [v7]
On Wed, 29 May 2024 14:19:52 GMT, Viktor Klang wrote:
>> Doug Lea has updated the pull request with a new target base due to a merge
>> or a rebase. The incremental webrev excludes the unrelated changes brought
>> in by the merge/rebase. The pull request contains 41 additional commits
>> since the last revision:
>>
>> - Merge branch 'openjdk:master' into JDK-8322732
>> - Add test for utilization with interdependent tasks
>> - Un-misplace onSpinWait call
>> - Adjust control flow
>> - Reduce memory stalls
>> - Merge branch 'openjdk:master' into JDK-8322732
>> - More performance tradoffs
>> - Address review comments
>> - Merge branch 'openjdk:master' into JDK-8322732
>> - Repack some fields; adjust control flow
>> - ... and 31 more: https://git.openjdk.org/jdk/compare/2d88272f...cf5fe55c
>
> src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 2094:
>
>> 2092: if (((p = w.phase) & IDLE) != 0)
>> 2093: p = awaitWork(w, delay); // block, drop,
>> or exit
>> 2094: }
>
> I'm presuming the code below would be equivalent but avoid calculating the
> delay unless w.phase is IDLE?
>
> Suggestion:
>
> if ((p & IDLE) != 0 && ((p = w.phase) & IDLE) != 0) {
> long delay = (((qc & RC_MASK) > 0L) ? 0L :
> (w.source != INVALID_ID) ? keepAlive :
> TIMEOUT_SLOP); // minimal delay if
> cascade
> p = awaitWork(w, delay); // block, drop, or exit
> }
Seems slightly better not to recheck until call, since previous loop would
usually have caught this if there was actually any work.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19131#discussion_r1619034460
Re: RFR: 8322732: ForkJoinPool may underutilize cores in async mode [v7]
On Wed, 29 May 2024 13:26:10 GMT, Viktor Klang wrote:
>> Doug Lea has updated the pull request with a new target base due to a merge
>> or a rebase. The incremental webrev excludes the unrelated changes brought
>> in by the merge/rebase. The pull request contains 41 additional commits
>> since the last revision:
>>
>> - Merge branch 'openjdk:master' into JDK-8322732
>> - Add test for utilization with interdependent tasks
>> - Un-misplace onSpinWait call
>> - Adjust control flow
>> - Reduce memory stalls
>> - Merge branch 'openjdk:master' into JDK-8322732
>> - More performance tradoffs
>> - Address review comments
>> - Merge branch 'openjdk:master' into JDK-8322732
>> - Repack some fields; adjust control flow
>> - ... and 31 more: https://git.openjdk.org/jdk/compare/c056aa9c...cf5fe55c
>
> src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 912:
>
>> 910: * enough to avoid resizing in most tree-structured tasks, but
>> 911: * larger for external queues where both false-sharing problems
>> 912: * and the need for resizing are more common.. (Maintenance note:
>
> Suggestion:
>
> * and the need for resizing are more common. (Maintenance note:
thanks, done.
> src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 1857:
>
>> 1855:(LMASK & c);
>> 1856: }
>> 1857: if ((tryTerminate(false, false) & STOP) == 0 && w != null) {
>
> Suggestion:
>
> if ((tryTerminate(false, false) & STOP) == 0L && w != null) {
yes.
> src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 2061:
>
>> 2059: w.stackPred = (int)pc; // set ctl stack link
>> 2060: qc = (active & LMASK) | ((pc - RC_UNIT) & UMASK);
>> 2061: if (pc == (pc = compareAndExchangeCtl(pc, qc)))
>
> If I'm reading this right you might want to add a comment :)
>
> Suggestion:
>
> if (pc == (pc = compareAndExchangeCtl(pc, qc))) // consistent
thanks, done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19131#discussion_r1619023295
PR Review Comment: https://git.openjdk.org/jdk/pull/19131#discussion_r1619024417
PR Review Comment: https://git.openjdk.org/jdk/pull/19131#discussion_r1619025818
Re: RFR: 8322732: ForkJoinPool may underutilize cores in async mode [v7]
On Wed, 29 May 2024 11:33:40 GMT, Doug Lea wrote:
>> This set of changes address causes of poor utilization with small numbers of
>> cores due to overly aggressive contention avoidance. A number of further
>> adjustments were needed to still avoid most contention effects in
>> deployments with large numbers of cores
>
> Doug Lea has updated the pull request with a new target base due to a merge
> or a rebase. The incremental webrev excludes the unrelated changes brought in
> by the merge/rebase. The pull request contains 41 additional commits since
> the last revision:
>
> - Merge branch 'openjdk:master' into JDK-8322732
> - Add test for utilization with interdependent tasks
> - Un-misplace onSpinWait call
> - Adjust control flow
> - Reduce memory stalls
> - Merge branch 'openjdk:master' into JDK-8322732
> - More performance tradoffs
> - Address review comments
> - Merge branch 'openjdk:master' into JDK-8322732
> - Repack some fields; adjust control flow
> - ... and 31 more: https://git.openjdk.org/jdk/compare/1dc80514...cf5fe55c
src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 2094:
> 2092: if (((p = w.phase) & IDLE) != 0)
> 2093: p = awaitWork(w, delay); // block, drop, or
> exit
> 2094: }
I'm presuming the code below would be equivalent but avoid calculating the
delay unless w.phase is IDLE?
Suggestion:
if ((p & IDLE) != 0 && ((p = w.phase) & IDLE) != 0) {
long delay = (((qc & RC_MASK) > 0L) ? 0L :
(w.source != INVALID_ID) ? keepAlive :
TIMEOUT_SLOP); // minimal delay if cascade
p = awaitWork(w, delay); // block, drop, or exit
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19131#discussion_r1618980694
Re: RFR: 8322732: ForkJoinPool may underutilize cores in async mode [v7]
On Wed, 29 May 2024 11:33:40 GMT, Doug Lea wrote: >> This set of changes address causes of poor utilization with small numbers of >> cores due to overly aggressive contention avoidance. A number of further >> adjustments were needed to still avoid most contention effects in >> deployments with large numbers of cores > > Doug Lea has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 41 additional commits since > the last revision: > > - Merge branch 'openjdk:master' into JDK-8322732 > - Add test for utilization with interdependent tasks > - Un-misplace onSpinWait call > - Adjust control flow > - Reduce memory stalls > - Merge branch 'openjdk:master' into JDK-8322732 > - More performance tradoffs > - Address review comments > - Merge branch 'openjdk:master' into JDK-8322732 > - Repack some fields; adjust control flow > - ... and 31 more: https://git.openjdk.org/jdk/compare/3d109313...cf5fe55c src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 2061: > 2059: w.stackPred = (int)pc; // set ctl stack link > 2060: qc = (active & LMASK) | ((pc - RC_UNIT) & UMASK); > 2061: if (pc == (pc = compareAndExchangeCtl(pc, qc))) If I'm reading this right you might want to add a comment :) Suggestion: if (pc == (pc = compareAndExchangeCtl(pc, qc))) // consistent - PR Review Comment: https://git.openjdk.org/jdk/pull/19131#discussion_r1618974704
Re: RFR: 8322732: ForkJoinPool may underutilize cores in async mode [v7]
On Wed, 29 May 2024 11:33:40 GMT, Doug Lea wrote: >> This set of changes address causes of poor utilization with small numbers of >> cores due to overly aggressive contention avoidance. A number of further >> adjustments were needed to still avoid most contention effects in >> deployments with large numbers of cores > > Doug Lea has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 41 additional commits since > the last revision: > > - Merge branch 'openjdk:master' into JDK-8322732 > - Add test for utilization with interdependent tasks > - Un-misplace onSpinWait call > - Adjust control flow > - Reduce memory stalls > - Merge branch 'openjdk:master' into JDK-8322732 > - More performance tradoffs > - Address review comments > - Merge branch 'openjdk:master' into JDK-8322732 > - Repack some fields; adjust control flow > - ... and 31 more: https://git.openjdk.org/jdk/compare/01d11057...cf5fe55c src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 1969: > 1967: else if ((e & SHUTDOWN) == 0) > 1968: return 0; > 1969: else if (compareAndSetCtl(c, c) && casRunState(e, e | > STOP)) is the `compareAndSetCtl(c, c)` needed for the read+write fence if ctl == c? - PR Review Comment: https://git.openjdk.org/jdk/pull/19131#discussion_r1618963723
Re: RFR: 8322732: ForkJoinPool may underutilize cores in async mode [v7]
On Wed, 29 May 2024 11:33:40 GMT, Doug Lea wrote:
>> This set of changes address causes of poor utilization with small numbers of
>> cores due to overly aggressive contention avoidance. A number of further
>> adjustments were needed to still avoid most contention effects in
>> deployments with large numbers of cores
>
> Doug Lea has updated the pull request with a new target base due to a merge
> or a rebase. The incremental webrev excludes the unrelated changes brought in
> by the merge/rebase. The pull request contains 41 additional commits since
> the last revision:
>
> - Merge branch 'openjdk:master' into JDK-8322732
> - Add test for utilization with interdependent tasks
> - Un-misplace onSpinWait call
> - Adjust control flow
> - Reduce memory stalls
> - Merge branch 'openjdk:master' into JDK-8322732
> - More performance tradoffs
> - Address review comments
> - Merge branch 'openjdk:master' into JDK-8322732
> - Repack some fields; adjust control flow
> - ... and 31 more: https://git.openjdk.org/jdk/compare/f715b3e6...cf5fe55c
src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 1857:
> 1855:(LMASK & c);
> 1856: }
> 1857: if ((tryTerminate(false, false) & STOP) == 0 && w != null) {
Suggestion:
if ((tryTerminate(false, false) & STOP) == 0L && w != null) {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19131#discussion_r1618956269
Re: RFR: 8322732: ForkJoinPool may underutilize cores in async mode [v7]
On Wed, 29 May 2024 11:33:40 GMT, Doug Lea wrote:
>> This set of changes address causes of poor utilization with small numbers of
>> cores due to overly aggressive contention avoidance. A number of further
>> adjustments were needed to still avoid most contention effects in
>> deployments with large numbers of cores
>
> Doug Lea has updated the pull request with a new target base due to a merge
> or a rebase. The incremental webrev excludes the unrelated changes brought in
> by the merge/rebase. The pull request contains 41 additional commits since
> the last revision:
>
> - Merge branch 'openjdk:master' into JDK-8322732
> - Add test for utilization with interdependent tasks
> - Un-misplace onSpinWait call
> - Adjust control flow
> - Reduce memory stalls
> - Merge branch 'openjdk:master' into JDK-8322732
> - More performance tradoffs
> - Address review comments
> - Merge branch 'openjdk:master' into JDK-8322732
> - Repack some fields; adjust control flow
> - ... and 31 more: https://git.openjdk.org/jdk/compare/2d55c424...cf5fe55c
src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 1452:
> 1450: * Runs the given task, as well as remaining local tasks.
> 1451: */
> 1452: final void topLevelExec(ForkJoinTask task, int cfg) {
Looks real clean, nice work @DougLea
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19131#discussion_r1618948755
Re: RFR: 8322732: ForkJoinPool may underutilize cores in async mode [v7]
On Wed, 29 May 2024 11:33:40 GMT, Doug Lea wrote: >> This set of changes address causes of poor utilization with small numbers of >> cores due to overly aggressive contention avoidance. A number of further >> adjustments were needed to still avoid most contention effects in >> deployments with large numbers of cores > > Doug Lea has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 41 additional commits since > the last revision: > > - Merge branch 'openjdk:master' into JDK-8322732 > - Add test for utilization with interdependent tasks > - Un-misplace onSpinWait call > - Adjust control flow > - Reduce memory stalls > - Merge branch 'openjdk:master' into JDK-8322732 > - More performance tradoffs > - Address review comments > - Merge branch 'openjdk:master' into JDK-8322732 > - Repack some fields; adjust control flow > - ... and 31 more: https://git.openjdk.org/jdk/compare/9388a872...cf5fe55c src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 912: > 910: * enough to avoid resizing in most tree-structured tasks, but > 911: * larger for external queues where both false-sharing problems > 912: * and the need for resizing are more common.. (Maintenance note: Suggestion: * and the need for resizing are more common. (Maintenance note: - PR Review Comment: https://git.openjdk.org/jdk/pull/19131#discussion_r1618884462
Re: RFR: 8322732: ForkJoinPool may underutilize cores in async mode [v7]
> This set of changes address causes of poor utilization with small numbers of > cores due to overly aggressive contention avoidance. A number of further > adjustments were needed to still avoid most contention effects in deployments > with large numbers of cores Doug Lea has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 41 additional commits since the last revision: - Merge branch 'openjdk:master' into JDK-8322732 - Add test for utilization with interdependent tasks - Un-misplace onSpinWait call - Adjust control flow - Reduce memory stalls - Merge branch 'openjdk:master' into JDK-8322732 - More performance tradoffs - Address review comments - Merge branch 'openjdk:master' into JDK-8322732 - Repack some fields; adjust control flow - ... and 31 more: https://git.openjdk.org/jdk/compare/cc3280e5...cf5fe55c - Changes: - all: https://git.openjdk.org/jdk/pull/19131/files - new: https://git.openjdk.org/jdk/pull/19131/files/a262f6c6..cf5fe55c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19131&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19131&range=05-06 Stats: 23145 lines in 634 files changed: 14865 ins; 4679 del; 3601 mod Patch: https://git.openjdk.org/jdk/pull/19131.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19131/head:pull/19131 PR: https://git.openjdk.org/jdk/pull/19131
