daniellansun commented on PR #2986: URL: https://github.com/apache/groovy/pull/2986#issuecomment-6392508175
@paulk-asert Thank you for the reproduction and for splitting the two halves. The stall is real, and the tables are convincing: `minimumRunnable` alone and `managedBlock` alone do not fix it, and together they do. They also show the cost. I have taken the suggestion and removed that compensation rather than shipping it. `Pool.fixed(n)` is a `ForkJoinPool` of parallelism `n` again (`new ForkJoinPool(n)`, and the same for `Pool.cpu()`). Parallel collection callbacks on a fork-join pool are ordinary stream tasks. They are not wrapped in `managedBlock`, so a CPU-bound body does not recruit spare workers, and the common pool is not enlarged. On the compensated build, JDK 25, `Pool.fixed(2)` over a CPU `forEach` grew to 6 threads; after the removal the same shape stays at 2. Users who pick `fixed(n)` to cap CPU use keep that cap. `maximumPoolSize` and `minimumRunnable` are the JDK defaults again, including for async `Pool.fixed` / `Pool.cpu` callers. The JIRA description still talks about the earlier queue-check split and does not mention this pool-constructor change; that text needs a human update once the approach is settled. I have not edited the ticket. Blocking on a fork-join pool can still stall when every worker blocks with a sibling queued. The flaky tests now do what you suggested: `ParallelScope.withPool(parties + 1)`, with the pool created outside the scope and closed afterwards. They assert `parallelism == threads` and `poolSize <= threads`, and an upper bound on worker names. Twenty rounds are a concurrency check. They are not a statistical proof at one stall in 15k–80k rounds, and they no longer treat pool growth as expected. A separate CPU test locks the cap. I could not reproduce the original stall on JDK 25 either. I did not re-run your JDK 17 and 21 matrices, so those results stand. For a blocking workload the opt-in is `Pool.virtual()`, and `Pool.io()` where that is the virtual pool. `asForkJoinPool()` still throws for those pools. Parallel collection methods catch that `UnsupportedOperationException` and submit one task per element to that executor. They do not fall back to the common pool. Any other exception from `asForkJoinPool()` propagates. On JDKs without virtual threads, `Pool.io()` remains `fixed(ConcurrentConfig.defaultParallelism())` and the spare-worker rule applies. I confirmed that JDK 17 branch by reading `DefaultPool`, not by running it: this machine is JDK 25, and the `Pool.io()` test took the virtual-thread path. The docs no longer say that a pool of size `n` can host `n` concurrent blockers. They say a fork-join pool's parallelism is a hard cap, and that waiting work belongs on `virtual()` / `io()`. Two smaller notes from the review are fixed with the same change. There is one executor wait path, and it restores the interrupt flag on every wait; the old path restored it only when compensation ran. A synchronous executor runs the task on the caller and leaves a caller interrupt set. A pool thread drops a cancellation interrupt left by the previous task, and the worker slot and the interrupt are taken under one lock so that interrupt cannot land on the next task that reused the thread. `grepParallel` builds the `isCase` predicate outside the task. `collectManyParallel` applies the transform inside the stream or flat-map task. They are no longer wrapped in different places. Nested parallel calls stay on the current pool. A fork-join leaf does not inherit the submitter's binding, so each callback rebinds that pool on the worker that runs it. On a bounded executor that does not grow, a nested call runs on the worker already executing the outer element, which keeps that worker from waiting on a task queued behind it. `Pool.virtual()` still runs each element as its own task. A null element, a null reduction, and a null group key now behave the same on a fork-join pool and on `Pool.virtual()`. `findParallel` and `findAnyParallel` no longer finish through `Optional.of`, which threw when the match was null. `minParallel`, `maxParallel`, and `sumParallel` fold in encounter order and can return null; a one-element collection does not call the comparator or operator. `groupByParallel` keeps a null key, and each group stays in encounter order, on both paths. I would be glad of a look at the revised contract, especially if CI was on a JDK other than 25. -- 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]
