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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
