8359936: StableValues can release the underlying function after complete computation #25878
Conversation
|
👋 Welcome back pminborg! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
Webrevs
|
| preventReentry(); | ||
| synchronized (this) { | ||
| final Object t = contents; // Plain semantics suffice here | ||
| if (t == null) { | ||
| final T newValue = supplier.get(); | ||
| // The mutex is not reentrant so we know newValue should be returned | ||
| wrapAndSet(newValue); | ||
| if (underlyingHolder != null) { |
There was a problem hiding this comment.
Under what circumstances can the underlyingHolder be null here?
There was a problem hiding this comment.
If we call this method via the public StableValue::orElseSet, the underlying holder will be null. In this case, there is no underlying function stored. Instead, it is typically a lambda or an anonymous class provided on the fly.
| this.counter = counter; | ||
| // Safe publication | ||
| UNSAFE.storeStoreFence(); |
There was a problem hiding this comment.
This may actually be substitutable by making the initial write to counter a volatile write:
| this.counter = counter; | |
| // Safe publication | |
| UNSAFE.storeStoreFence(); | |
| Unsafe.putIntVolatile(this, COUNTER_OFFSET, counter); // Safe publication of underlying and counter |
There was a problem hiding this comment.
True. We could also use piggybacking.
|
|
||
| @ForceInline | ||
| @Override | ||
| public T get() { | ||
| return delegate.orElseSet(original); | ||
| return delegate.orElseSet(underlyingHolder.underlying(), underlyingHolder); |
There was a problem hiding this comment.
By avoiding reading from underlyingHolder when the StableValue is already set, this can avoid the overhead of a mutable field read from underlyingHolder:
| return delegate.orElseSet(underlyingHolder.underlying(), underlyingHolder); | |
| final Object t = delegate.wrappedContentsAcquire(); | |
| if (t != null) { | |
| return unwrap(t); | |
| } | |
| return delegate.orElseSet(underlyingHolder.underlying(), underlyingHolder); |
There was a problem hiding this comment.
Before this proposal:
Benchmark Mode Cnt Score Error Units
StableSupplierBenchmark.stable avgt 10 1.369 ± 0.019 ns/op
StableSupplierBenchmark.staticStable avgt 10 0.343 ± 0.005 ns/op
StableSupplierBenchmark.staticSupplier avgt 10 0.389 ± 0.008 ns/op
StableSupplierBenchmark.supplier avgt 10 1.739 ± 0.047 ns/op
After this proposal:
Benchmark Mode Cnt Score Error Units
StableSupplierBenchmark.stable avgt 10 1.436 ± 0.120 ns/op
StableSupplierBenchmark.staticStable avgt 10 0.352 ± 0.044 ns/op
StableSupplierBenchmark.staticSupplier avgt 10 0.346 ± 0.016 ns/op
StableSupplierBenchmark.supplier avgt 10 1.588 ± 0.035 ns/op. <-- !
So, it appears to be some gain here.
|
Performance looks really good on all platforms except Linux AArch64, where there are significant regressions for:
These need to be addressed before integration. |
This PR proposes to release the underlying function if a stable function or collection has invoked its underlying supplier exhaustively so that it can be collected.
This PR passes tier1, tier2, and tier3 testing on multiple platforms.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25878/head:pull/25878$ git checkout pull/25878Update a local copy of the PR:
$ git checkout pull/25878$ git pull https://git.openjdk.org/jdk.git pull/25878/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25878View PR using the GUI difftool:
$ git pr show -t 25878Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25878.diff
Using Webrev
Link to Webrev Comment