RFR: 8330465: Stable Values and Collections (Internal)

[ExE Boss]

On Tue, 16 Apr 2024 11:47:23 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 an `AtomicReference` and one protected by double-checked locking under concurrent access by 8 threads:
> 
> 
> Benchmark                       Mode  Cnt  Score   Error  Units
> StableBenchmark.instanceAtomic  avgt   10  1.576 ? 0.052  ns/op
> StableBenchmark.instanceDCL     avgt   10  1.608 ? 0.059  ns/op
> StableBenchmark.instanceStable  avgt   10  0.979 ? 0.023  ns/op <- StableValue (~40% faster than DCL)
> 
> 
> Performance compared to static variables protected by `AtomicReference`, class-holder idiom holder, and double-checked locking (8 threads):
> 
> 
> Benchmark                       Mode  Cnt  Score   Error  Units
> StableBenchmark.staticAtomic    avgt   10  1.335 ? 0.056  ns/op
> StableBenchmark.staticCHI       avgt   10  0.623 ? 0.086  ns/op
> StableBenchmark.staticDCL       avgt   10  1.418 ? 0.171  ns/op
> StableBenchmark.staticList      avgt   10  0.617 ? 0.024  ns/op
> StableBenchmark.staticStable    avgt   10  0.604 ? 0.022  ns/op <- StableValue ( > 2x faster than `AtomicInteger` and DCL)
> 
> 
> Performance for stable lists in both instance and static contexts whereby the sum of random contents is calculated for stable lists (which are thread-safe) compared to `ArrayList` instances (which are not thread-safe) (under single thread access):
> 
> 
> Benchmark                                     Mode  Cnt  Score   Error  Units
> StableListSumBenchmark.instanceArrayList      avgt   10  0.356 ? 0.005  ns/op
> StableListSumBenchmark.instanceList           avgt   10  0.373 ? 0.017  ns/op <- Stable list
> StableListSumBenchmark.staticArrayList        avgt   10  0.352 ? 0.002  ns/op
> StableListSumBenchmark.staticList             avgt   10  0.356 ? 0.00...

**Nit:** Inconsistent whitespace:

src/java.base/share/classes/java/lang/reflect/AccessibleObject.java line 393:

> 391:     }
> 392: 
> 393:     InaccessibleObjectException newInaccessibleObjectException(String msg) {

This internal helper method can be `static`:
Suggestion:

    static InaccessibleObjectException newInaccessibleObjectException(String msg) {

src/java.base/share/classes/jdk/internal/lang/stable/StableValueElement.java line 57:

> 55:         return switch (stateVolatile()) {
> 56:             case UNSET    -> throw new NoSuchElementException(); // No value was set
> 57:             case NON_NULL -> orThrowVolatile(); // Race: another thread has set a value

It should be safe to avoid self‑recursion here:
Suggestion:

            case NON_NULL -> elements[index]; // Race: another thread has set a value

or:
Suggestion:

            case NON_NULL -> {
                v = elements[index]; // Race: another thread has set a value
                if (v != null) {
                    yield v;
                }
                throw shouldNotReachHere();
            }

src/java.base/share/classes/jdk/internal/lang/stable/StableValueElement.java line 63:

> 61:         // more compact byte code.
> 62:         switch (stateVolatile()) {
> 63:             case UNSET:    { throw StableUtil.notSet();}

Suggestion:

            case UNSET:    { throw StableUtil.notSet(); }

src/java.base/share/classes/jdk/internal/lang/stable/StableValueElement.java line 116:

> 114:     public V computeIfUnset(Supplier<? extends V> supplier) {
> 115:         // Todo: This creates a lambda
> 116:         return computeIfUnsetShared(supplier, Supplier::get);

Suggestion:

        return computeIfUnsetShared(supplier, supplierExtractor());

src/java.base/share/classes/jdk/internal/lang/stable/StableValueElement.java line 144:

> 142:         // more compact byte code.
> 143:         switch (stateVolatile()) {
> 144:             case UNSET:    { return computeIfUnsetVolatile0(provider, key);}

Suggestion:

            case UNSET:    { return computeIfUnsetVolatile0(provider, key); }

src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line 116:

> 114:         // more compact byte code.
> 115:         switch (stateVolatile()) {
> 116:             case UNSET:    { throw StableUtil.notSet();}

Suggestion:

            case UNSET:    { throw StableUtil.notSet(); }

src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line 181:

> 179:         // more compact byte code.
> 180:         switch (stateVolatile()) {
> 181:             case UNSET:    { return computeIfUnsetVolatile0(supplier);}

Suggestion:

            case UNSET:    { return computeIfUnsetVolatile0(supplier); }

-------------

PR Review: https://git.openjdk.org/jdk/pull/18794#pullrequestreview-2029063641
PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1570620018
PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1580882515
PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1583418107
PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1572101643
PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1583418492
PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1583419719
PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1583420000