[
https://issues.apache.org/jira/browse/GROOVY-12457?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18128891#comment-18128891
]
ASF GitHub Bot commented on GROOVY-12457:
-----------------------------------------
paulk-asert commented on PR #2986:
URL: https://github.com/apache/groovy/pull/2986#issuecomment-6276568775
AI read:
> ````markdown
> The stall is real. I reproduced it in plain Java (no Groovy): a `new
ForkJoinPool(2)`, a 2-element `parallelStream()` and a timed
`CyclicBarrier(2).await` on each element. I also checked which half of the
change is doing the work:
>
> | JDK | plain pool | `minimumRunnable` only | `managedBlock` wrap only |
both (this PR) | plain pool, size 3 |
> |---|---|---|---|---|---|
> | 17 | 3 stalls / 100k rounds | 0 / 30k | 1 / 30k | 0 / 200k | 0 / 100k |
> | 21 | 2 stalls / ~16k rounds | 2 / 30k | 0 / 30k | 0 / 200k | 0 / 100k |
> | 25 | 0 / 33k | 0 / 30k | 0 / 30k | 0 / 30k | – |
>
> So the combination does fix it, and each half on its own does not. But I
don't think we should merge it in its current form:
>
> 1. **`Pool.fixed(n)` stops limiting parallelism.** The wrapper can't tell
whether a callback blocks, so it compensates on every call that has a queued
sibling. That includes CPU-bound bodies and cheap comparators. CPU-bound
`forEach` over 200k elements, JDK 21, 12 cores:
>
> | pool | threads (before → after) | best time (before → after) |
> |---|---|---|
> | `fixed(1)` | 1 → 3 | 1604 → 794 ms |
> | `fixed(2)` | 2 → 6 | 799 → 406 ms |
> | `fixed(4)` | 4 → 11 | 404 → 221 ms |
> | `fixed(8)` | 8 → 19 | 233 → 191 ms |
>
> The time roughly halving shows the extra threads really run
concurrently. Users who pick `fixed(n)` to cap CPU use lose that cap. Even a
trivial body went from 4 to 10 threads.
> 2. **The default path changes the common pool.** With no `ParallelScope`,
the callbacks run on `ForkJoinPool.commonPool()`, which grew from 11 to 15
threads in the same test. That is JVM-wide.
> 3. **The `DefaultPool` change reaches beyond parallel collections.**
`maximumPoolSize` goes from the JDK default (`n + 256`) to `Integer.MAX_VALUE`,
and `minimumRunnable = n` affects every `Pool.fixed` / `Pool.cpu` user,
including async code. The JIRA description doesn't mention this change. It also
still describes the earlier JDK 20+ / JDK 17 queue-check split rather than the
final `ForkJoinTask.getQueuedTaskCount()` approach.
> 4. **The `Pool.io()` / `Pool.virtual()` advice doesn't hold for parallel
collections.** `asForkJoinPool()` throws for the virtual-thread executor, so
`withCurrentFJP` silently falls back to the common pool. `Pool.io()` is
`virtual()` on JDK 21+ and `fixed(...)` before that, so the same script gets an
isolated pool on 17 and the shared pool on 21+. That fallback predates this PR,
but the new docs send blocking workloads straight to it. The "a pool of size
`n` can host `n` concurrent blockers" wording also no longer matches what the
pool does, since it can grow past `n`.
> 5. **Tests:** at 1 stall in roughly 15k–80k rounds, 20 rounds will almost
always pass on master, so they don't really guard the fix. The relaxed `>=
parties` assertion also makes the pool growth look like expected behaviour.
>
> Minor: `InterruptedException` re-sets the interrupt flag only on the
compensated path. `collectManyParallel` / `grepParallel` wrap the callback
inside the submitted task, while the other methods wrap it outside.
>
> **Suggestion:** for the flaky tests, `ParallelScope.withPool(parties + 1)`
is enough and needs no product change (0 stalls in 100k rounds on 17 and 21).
For a product fix, I'd prefer compensation that users opt into for blocking
work, for example by making `Pool.io()` / `Pool.virtual()` actually work for
parallel collections (or rejecting them clearly) instead of falling back to the
common pool. That keeps `fixed(n)` limited to `n` threads. Which JDK was the CI
flake on? I couldn't reproduce it on 25, though that's weak evidence.
> ````
>
> The `Pool.io()` point about JDK 17 getting its own pool comes from reading
the source; I didn't run it through Groovy.
> Blocking parallel-collection callbacks can fail to run a queued sibling task
> ----------------------------------------------------------------------------
>
> Key: GROOVY-12457
> URL: https://issues.apache.org/jira/browse/GROOVY-12457
> Project: Groovy
> Issue Type: Bug
> Reporter: Daniel Sun
> Priority: Major
>
> h2. Context
> {{{}eachParallel{}}}, {{{}collectParallel{}}}, and the other parallel
> collection methods run their callback on a {{{}ForkJoinPool{}}}: the pool
> bound by {{{}ParallelScope.withPool{}}}, or {{ForkJoinPool.commonPool()}}
> when no pool is bound. {{@Parallel}} {{for-in}} loops are rewritten to call
> {{{}eachParallel{}}}. The methods arrived with GROOVY-11952
> ([GEP-18|https://groovy.apache.org/wiki/GEP-18.html]).
> A pool of size {{n}} is the intended way to run {{n}} callbacks that block at
> the same time.
> h2. Problem
> This rendezvous fails intermittently, including when the pool size equals the
> number of parties:
> {code:groovy}
> import groovy.concurrent.ParallelScope
> import java.util.concurrent.CyclicBarrier
> import java.util.concurrent.TimeUnit
> int parties = 2
> ParallelScope.withPool(parties) { scope ->
> def barrier = new CyclicBarrier(parties)
> (1..parties).eachParallel {
> barrier.await(5, TimeUnit.SECONDS)
> }
> }
> {code}
> {{CyclicBarrier.await}} throws {{TimeoutException}} because the second
> element never starts. The failure is reported as a nested
> {{{}UndeclaredThrowableException{}}}:
> ||Layer||What the caller sees||
> |Closure|{{TimeoutException}} from {{CyclicBarrier.await}}|
> |{{Consumer}} proxy|{{{}UndeclaredThrowableException{}}}, because {{accept}}
> does not declare checked exceptions|
> |{{ForkJoinTask.getThrowableException}}|A second
> {{{}UndeclaredThrowableException{}}}, rebuilt on the joining thread|
> The same shape fails for {{@Parallel}} {{{}for (item in 1..n){}}}. It shows
> up more often once the pool's workers have already run and parked. A freshly
> created pool can fail as well.
> h2. Cause
> {{parallelStream()}} forks the sibling leaf onto the current worker's queue,
> then runs one leaf on that same worker. {{CyclicBarrier.await}} is not a
> {{{}ForkJoinPool.ManagedBlocker{}}}, so the pool does not release or replace
> the blocked worker. If the other worker has already scanned and parked, the
> fork signal is not always delivered again. The sibling stays on the queue
> until the barrier times out.
> Sizing the pool to exactly {{n}} threads does not close the race. The idle
> thread is there; it is parked.
> h2. Change
> The proposed change runs the user callback inside
> {{ForkJoinPool.managedBlock}} when the current worker still has queued tasks.
> That releases a parked worker, or creates a spare, before the callback can
> block. A leaf whose queue is empty is unchanged.
> * JDK 20 and later read {{{}ForkJoinWorkerThread.getQueuedTaskCount(){}}}.
> * JDK 17 does not have that method, so the check uses the pool-wide
> queued-task estimate.
> The wrapper covers every parallel-collection callback, including
> {{eachWithIndexParallel}} and the {{eachParallel}} call emitted for
> {{{}@Parallel{}}}.
> Compensation runs only when a sibling is already queued. A callback that
> blocks before any sibling has been forked can still stall, and so can a pool
> that refuses another thread. Work that spends most of its time waiting still
> fits {{Pool.io()}} or {{Pool.virtual()}} better than a fixed
> {{{}ForkJoinPool{}}}.
> h2. Tests
> * {{ParallelCollectionTest}} and {{ParallelAnnotationTest}} repeat the
> two-party barrier on one pool, so later rounds run against workers parked by
> the first round. The barrier must trip.
> * The thread-count assertion allows a spare ({{{}size() >= parties{}}}),
> because {{managedBlock}} may add one while a worker is blocked.
> * The full two test classes passed on JDK 25. The three barrier tests also
> passed on JDK 17, which exercises the queue-length fallback.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
