On Wed, 15 May 2024 15:27:34 GMT, Per Minborg <pminb...@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
