RFR: 8330465: Stable Values and Collections (Internal) [v5]
Chen Liang
liach at openjdk.org
Wed May 15 16:36:10 UTC 2024
On Wed, 15 May 2024 15:27:34 GMT, Per Minborg <pminborg at openjdk.org> wrote:
>> # Stable Values & Collections (Internal)
>>
>> ## Summary
>> This PR proposes to introduce an internal _Stable Values & Collections_ API, which provides immutable value holders where elements are initialized _at most once_. Stable Values & Collections offer the performance and safety benefits of final fields while offering greater flexibility as to the timing of initialization.
>>
>> ## Goals
>> * Provide an easy and intuitive API to describe value holders that can change at most once.
>> * Decouple declaration from initialization without significant footprint or performance penalties.
>> * Reduce the amount of static initializer and/or field initialization code.
>> * Uphold integrity and consistency, even in a multi-threaded environment.
>>
>> For more details, see the draft JEP: https://openjdk.org/jeps/8312611
>>
>> ## Performance
>> Performance compared to instance variables using two `AtomicReference` and two protected by double-checked locking under concurrent access by all threads:
>>
>>
>> Benchmark Mode Cnt Score Error Units
>> StableBenchmark.atomic thrpt 10 259.478 ? 36.809 ops/us
>> StableBenchmark.dcl thrpt 10 225.710 ? 26.638 ops/us
>> StableBenchmark.stable thrpt 10 4382.478 ? 1151.472 ops/us <- StableValue significantly faster
>>
>>
>> Performance compared to static variables protected by `AtomicReference`, class-holder idiom holder, and double-checked locking (all threads):
>>
>>
>> Benchmark Mode Cnt Score Error Units
>> StableStaticBenchmark.atomic thrpt 10 6487.835 ? 385.639 ops/us
>> StableStaticBenchmark.dcl thrpt 10 6605.239 ? 210.610 ops/us
>> StableStaticBenchmark.stable thrpt 10 14338.239 ? 1426.874 ops/us
>> StableStaticBenchmark.staticCHI thrpt 10 13780.341 ? 1839.651 ops/us
>>
>>
>> Performance for stable lists (thread safe) in both instance and static contexts whereby we access a single value compared to `ArrayList` instances (which are not thread-safe) (all threads):
>>
>>
>> Benchmark Mode Cnt Score Error Units
>> StableListElementBenchmark.instanceArrayList thrpt 10 5812.992 ? 1169.730 ops/us
>> StableListElementBenchmark.instanceList thrpt 10 4818.643 ? 704.893 ops/us
>> StableListElementBenchmark...
>
> Per Minborg has updated the pull request incrementally with one additional commit since the last revision:
>
> Switch to monomorphic StableValue and use lazy arrays
src/java.base/share/classes/jdk/internal/lang/StableValue.java line 384:
> 382: * @param <T> the memoized type
> 383: */
> 384: static <T> Supplier<T> ofSupplier(Supplier<? extends T> original) {
`ofSupplier` sounds like this method returns a `StableValue` from a `Supplier`. I recommend another name, such as `stableSupplier`, `wrapSupplier`, or `memoize`, to better associate with the method's types.
src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line 75:
> 73: */
> 74: @Stable
> 75: private int state;
Can we change this to be a byte, so state and supplying fields can be packed together in 4 bytes in some layouts?
src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line 104:
> 102: // Optimistically try plain semantics first
> 103: final V v = value;
> 104: if (v != null) {
If `value == null && state == NULL`, can the path still be constant folded? I doubt it because `value` in this case may not be promoted to constant.
src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line 139:
> 137: case NON_NULL: { return valueVolatile(); }
> 138: case ERROR: { throw StableUtil.error(this); }
> 139: case DUMMY: { throw shouldNotReachHere(); }
Redundant branch?
src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line 236:
> 234: } catch (Throwable t) {
> 235: putState(ERROR);
> 236: putMutex(t.getClass());
Should we cache the exception instance so we can rethrow it in future ERROR state `orThrow` calls?
src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line 240:
> 238: }
> 239: } finally {
> 240: supplying = false;
Resetting a stable field is a bad idea. I recommend renaming this to `supplierCalled` or `supplied` so we never transition this false -> true
src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line 256:
> 254:
> 255: @ForceInline
> 256: private <K> V computeIfUnsetShared(Object provider, K key) {
Can we let suppliers share this path too, with a null key? I see this path supports suppliers but supplier code path doesn't call this path.
src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line 403:
> 401: stable.computeIfUnset(supplier);
> 402: } catch (Throwable throwable) {
> 403: final Thread.UncaughtExceptionHandler uncaughtExceptionHandler =
Does this exception handling differ from the default one for threads? If not, I think we can safely remove this catch block, as all exceptions are just propagated and computeIfUnset doesn't declare any checked exception.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1601935261
PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1601937748
PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1601916538
PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1601918911
PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1601940341
PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1601941518
PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1601942956
PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1601927751
More information about the hotspot-compiler-dev
mailing list