On Mon, 10 Mar 2025 18:11:23 GMT, Per Minborg <pminb...@openjdk.org> 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.
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`?
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()`
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)`.
src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line
168:
> 166: // Wraps `null` values into a sentinel value
> 167: @ForceInline
> 168: private static <T> T wrap(T t) {
Suggestion:
private static <T> Object wrap(T t) {
src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java line
181:
> 179: @SuppressWarnings("unchecked")
> 180: @ForceInline
> 181: private static <T> T nullSentinel() {
Suggestion:
private static Object nullSentinel() {
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r1988608920
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r1988612784
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r1993081551
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r1988616943
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r1993110162
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r1993111723
