RFR: 8351565: Implement JEP 502: Stable Values (Preview)
Quan Anh Mai
qamai at openjdk.org
Thu Mar 13 11:20:14 UTC 2025
On Mon, 10 Mar 2025 18:11:23 GMT, Per Minborg <pminborg at 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
More information about the core-libs-dev
mailing list