On Tue, 11 Mar 2025 07:50:38 GMT, Quan Anh Mai <[email protected]> wrote:
>> Implement JEP 502.
>>
>> The PR passes tier1-tier3 tests.
>
> src/java.base/share/classes/java/util/ImmutableCollections.java line 777:
>
>> 775: private final IntFunction<? extends E> mapper;
>> 776: @Stable
>> 777: private final StableValueImpl<E>[] backing;
>
> You can use a backing `@Stable Object[]` instead. It will reduce indirection
> when accessing this list.
Can you please elaborate a bit more on your proposal @merykitty?
> src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java
> line 65:
>
>> 63: //
>> 64: @Stable
>> 65: private volatile Object value;
>
> Can we use `acquire`/`release` semantics instead of `volatile`?
Yes we can. However, I am uncertain if the added complexity can motivate any
performance benefits. Perhaps on ARM? I can do a benchmark on it.
> src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java
> line 128:
>
>> 126: final T newValue = supplier.get();
>> 127: // The mutex is reentrant so we need to check if the value
>> was actually set.
>> 128: return wrapAndCas(newValue) ? newValue : orElseThrow();
>
> Reentrancy into here seems really buggy, I would endorse disallowing it
> instead. In that case, a `ReentrantLock` seems better than the native monitor
> as we can cheaply check `lock.isHeldByCurrentThread()`
StableValueImpl was carefully designed to minimize memory footprint. Adding a
lock would inflate memory usage substantially.
> src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java
> line 159:
>
>> 157: private boolean wrapAndCas(Object value) {
>> 158: // This upholds the invariant, a `@Stable` field is written to
>> at most once
>> 159: return UNSAFE.compareAndSetReference(this,
>> UNDERLYING_DATA_OFFSET, null, wrap(value));
>
> There is no need for a cas here as all setters have to hold the lock. We
> should have a dedicated private `set` that asserts `Thread.holdsLock(this)`.
This is more of a belt and suspenders solution. It is true that it is
redundant. A set volatile would suffice here.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r1989016337
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r1988630199
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r1993142117
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r1988634451
