8319447: Improve performance of delayed task handling

[DougLea]

(Copied from https://bugs.openjdk.org/browse/JDK-8319447)

The problems addressed by this CR/PR are that ScheduledThreadPoolExecutor is both ill-suited for many (if not most) of its applications, and is a performance bottleneck (as seen especially in Loom and CompletableFuture usages). After considering many options over the years, the approach taken here is to connect (lazily, only if used) a form of ScheduledExecutorService (DelayScheduler) to any ForkJoinPool (including the commonPool), which can then use more efficient and scalable techniques to request and trigger delayed actions, periodic actions, and cancellations, as well as coordinate shutdown and termination mechanics (see the internal documentation in DelayScheduler.java for algotihmic details). This speeds up some Loom operations by almost an order of magnitude (and similarly for CompletableFuture). Further incremental improvements may be possible, but delay scheduling overhead is now unlikely to be a common performance concern.

We also introduce method submitWithTimeout to schedule a timeout that cancels or otherwise completes a submitted task that takes too long. Support for this very common usage was missing from the ScheduledExecutorService API, and workarounds that users have tried are wasteful, often leaky, and error-prone. This cannot be added to the ScheduledExecutorService interface because it relies on ForkJoinTask methods (such as completeExceptionally) to be available in user-supplied timeout actions. The need to allow a pluggable handler reflects experience with the similar CompletableFuture.orTimeout, which users have found not to be flexible enough, so might be subject of future improvements.

A DelayScheduler is optionally (on first use of a scheduling method) constructed and started as part of a ForkJoinPool, not any other kind of ExecutorService. It doesn't make sense to do so with the other j.u.c pool implementation ThreadPoolExecutor. ScheduledThreadPoolExecutor already extends it in incompatible ways (which is why we can't just improve or replace STPE internals). However, as discussed in internal documentation, the implementation isolates calls and callbacks in a way that could be extracted out into (package-private) interfaces if another j.u.c pool type is introduced.

Only one of the policy controls in ScheduledThreadPoolExecutor applies to ForkJoinPools with DelaySchedulers: new method cancelDelayedTasksOnShutdown controls whether quiescent shutdown should wait for delayed tasks to become enabled and execute, or to cancel them and terminate. The default (to wait) matches default settings of STPE. Also new method getDelayedTaskCount allows monitoring.

We don't expect any compatibility issues: In the unlikely event that someone else has added the four SES methods to a ForkJoinPool subclass, they will continue to work as overrides. It's hard to imagine that anyone has added a form of submitWithTimeout with the same signature (Callable callable, long timeout, TimeUnit unit, Consumer<ForkJoinTask> timeoutAction), or methods with names cancelDelayedTasksOnShutdown or getDelayedTaskCount.

A snag: for reasons that should now be deprecated, ForkJoinPool allow users to externally (via properties) set the commonPool to zero parallelism, disabling worker thread creation, which makes no sense if a DelayScheduler is used. This property setting was made available to JavaEE frameworks so they could ensure that no new threads would be created in service of parallelStream operations (which are structured to be (slowly) executable single-threadedly via caller joins). However, no other async uses would work, which led to workarounds in CompletableFuture (also SubmissionPublisher) to handle this case by generating other threads. It was another arguably wrong decision to do this as well. A better solution that is backward compatible is to internally override commonPool parallelism zero only if known-async methods are used. This preserves original intent, and passes jtreg/tck tests that check for lack of thread creation in parallelStreams and related usages that don't otherwise use any async j.u.c components (although with some changes in unnecessarily implementation-dependent tests that made assumptions about exactly which threads/pools were used). It does require some changes in the wording of disclaimers in CompletableFuture and elsewhere though. And eventually, all this should go away, although it's not clear know how to deprecate a property setting. For now, the class-level javadoc has been updated to discourage use.

One remaining issue is whether to expose the underlying ScheduledForkJoinTask type. It currently isn't, in part because it would also require exposing currently non-public intervening classes (mainly FJT.InterruptibleTask). The main disadvantage with not exposing is that the schedule methods merely document that the returned ScheduledFuture is a ForkJoinTask rather than include this in signature. This could be revisited in the future without introducing incompatibilities (but with some internal implementation challenges to remove reliance on non-public-ness).

As minor follow-ups, we might expand use of DelaySchedulers internally in j.u.c classes to replace some timed waits.

Edit
Delete

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change requires CSR request JDK-8350493 to be approved

Issues

• JDK-8319447: Improve performance of delayed task handling (Enhancement - P4)
• JDK-8350493: Improve performance of delayed task handling (CSR)

Reviewers

• Alan Bateman (@AlanBateman - Reviewer) Review applies to b552c225
• Viktor Klang (@viktorklang-ora - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23702/head:pull/23702
$ git checkout pull/23702

Update a local copy of the PR:
$ git checkout pull/23702
$ git pull https://git.openjdk.org/jdk.git pull/23702/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23702

View PR using the GUI difftool:
$ git pr show -t 23702

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23702.diff

Using Webrev

Link to Webrev Comment

[viktorklang-ora]

@DougLea It would seem more consistent to have this return a long like getQueuedTaskCount (even if that amount isn't currently possible to represent in the heap. Thoughts, @AlanBateman?

[DougLea]

Arguably better, but not sure it is worth regenerating diffs for? (An int was used for the same reason as in FJT.getQueueSize -- they need to be valid array bounds. Which might someday allow long, but if so many things would change.)

[viktorklang-ora]

Personally I think it's worth it. @AlanBateman, what do you think?

[AlanBateman]

Right now it's hard to envisage needing to have >2B delayed tasks queued. Maybe in a few years we might regret this. So maybe better to change it to return long, I re-generate API diffs easily.

[DougLea]

OK. Changed to long. @AlanBateman could you regenerate specdiff after I commit?

[viktorklang-ora]

@DougLea Final review completed! I've added a few more comments and suggestions but after that I think this is ready to go. Thanks for the great work on this, Doug! /cc @AlanBateman

[scrhartley]

Spelling: innacurate -> inaccurate

[DougLea]

thanks; fixed.

[viktorklang-ora]

Thanks for all the hard work on this, Doug!

[DougLea]

/integrate

[openjdk]

Going to push as commit 8b0602d.
Since your change was applied there have been 84 commits pushed to the master branch:

• fe8bd75: 8351290: Clarify integral only for vector operators
• 4d1de46: 8352185: Shenandoah: Invalid logic for remembered set verification
• 3e96f5c: 8351366: Remove the java.security.debug=scl option
• 4247744: 8351435: Change the default Console implementation back to the built-in one in java.base module
• 9c06dcb: 8349583: Add mechanism to disable signature schemes based on their TLS scope
• cd5a43a: 8353126: Open source events tests batch1
• e4e6278: 8346129: Simplify EdDSA & XDH curve name usage
• 7a2e198: 8352277: java.security documentation: incorrect regex syntax describing "usage" algorithm constraint
• b7ca76e: 8353235: Test jdk/jfr/api/metadata/annotations/TestPeriod.java fails with IllegalArgumentException
• bbd5b17: 8339280: jarsigner -verify performs cross-checking between CEN and LOC
• ... and 74 more: https://git.openjdk.org/jdk/compare/dbc620fb1f754ca84f2a07abfdfbd4c5fcb55087...master

Your commit was automatically rebased without conflicts.

[openjdk]

@DougLea Pushed as commit 8b0602d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.