8351565: Implement JEP 502: Stable Values (Preview) #23972

minborg · 2025-03-10T18:11:23Z

Implement JEP 502.

The PR passes tier1-tier3 tests.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8342068 to be approved

Issues

JDK-8351565: Implement JEP 502: Stable Values (Preview) (Sub-task - P4)
JDK-8342068: Implement JEP 502: Stable Values (Preview) (CSR)

Reviewers

Viktor Klang (@viktorklang-ora - Reviewer)
Jorn Vernee (@JornVernee - Reviewer)
Alan Bateman (@AlanBateman - Reviewer)
Chen Liang (@liach - Reviewer)

Contributors

Maurizio Cimadamore <mcimadamore@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23972/head:pull/23972
$ git checkout pull/23972

Update a local copy of the PR:
$ git checkout pull/23972
$ git pull https://git.openjdk.org/jdk.git pull/23972/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23972

View PR using the GUI difftool:
$ git pr show -t 23972

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23972.diff

Using Webrev

Link to Webrev Comment

minborg · 2025-04-24T09:14:17Z

/reviewers 1

openjdk · 2025-04-24T09:14:56Z

@minborg
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 1 (with at least 1 Reviewer).

src/java.base/share/classes/java/util/ReverseOrderListView.java

src/java.base/share/classes/java/util/ImmutableCollections.java

viktorklang-ora

👍

lukellmann · 2025-04-24T13:35:45Z

src/java.base/share/classes/java/lang/StableValue.java

+ * Invocations of {@link #setOrThrow(Object)} form a total order of zero or more
+ * exceptional invocations followed by zero (if the contents were already set) or one
+ * successful invocation. Since stable functions and stable collections are built on top


How can an exceptional invocation of setOrThrow be followed by a successful invocation?

setOrThrow might be invoked several times. If the first invocation failed because the supplier threw and then the second invocation (with the same supplier) succeded (because there was some state that changed), we have this situation.

orElseSet is the method taking a supplier, setOrThrow doesn't.

Ahh. I'm sorry for misreading this. The docs should say orElseSet rather than orElseThrow. Again, we will fix this in a follow-up PR.

lukellmann · 2025-04-24T13:54:54Z

src/java.base/share/classes/java/lang/StableValue.java

+public sealed interface StableValue<T>
+        permits StableValueImpl {
+
+    // Principal methods


Not important, not practical and hacky (exceptions as control flow), but technically all methods could be implemented in terms of orElseSet, so they could also be called "convenience methods":

trySet(c) { success = false orElseSet(() -> {success = true; return c}) return succes } orElse(o) { try { return orElseSet(() -> throw) } catch { return o } } orElseThrow() { orElseSet(() -> throw) } isSet() { try { orElseSet(() -> throw) } catch { return false } return true } setOrThrow(c) { success = false orElseSet(() -> {success = true; return c}) if (!success) throw }

I guess these two comments (// Principal methods and // Convenience methods) could also just be omitted.

lukellmann · 2025-04-24T14:15:25Z

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

+    // Used to indicate a holder value is `null` (see field `value` below)
+    // A wrapper method `nullSentinel()` is used for generic type conversion.


Suggested change

// Used to indicate a holder value is `null` (see field `value` below)

// A wrapper method `nullSentinel()` is used for generic type conversion.

// Used to indicate a holder value is `null` (see field `contents` below)

The nullSentinel method no longer exists, the field is named contents.

lukellmann · 2025-04-24T14:19:02Z

src/java.base/share/classes/java/lang/StableValue.java

+     * @param contents to set
+     * @throws IllegalStateException if the contents was already set
+     */
+    void setOrThrow(T contents);


Should probably also mention that IllegalStateException can be caused by recursive initialization (see trySet).

lukellmann · 2025-04-24T14:22:19Z

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

+        // Prevent reentry via an orElseSet(supplier)
+        preventReentry();
+        // Mutual exclusion is required here as `orElseSet` might also
+        // attempt to modify the `wrappedValue`


Suggested change

// attempt to modify the `wrappedValue`

// attempt to modify `this.contents`

lukellmann · 2025-04-24T23:40:28Z

src/java.base/share/classes/java/util/ImmutableCollections.java

+                    return new NullableKeyValueHolder<>(k, inner.getValue().orElseSet(new Supplier<V>() {
+                        @Override public V get() { return mapper.apply(k); }}));


This lazy Entry implementation should then also have a toString that doesn't evaluate the contents. It should also be used in forEachRemaining.

lukellmann · 2025-04-24T23:53:11Z

src/java.base/share/classes/java/util/ImmutableCollections.java

+                final StableValueImpl<E>[] delegates = ((StableList<E>)base).delegates;
+                final StableValueImpl<E>[] reversed = ArraysSupport.reverse(
+                        Arrays.copyOf(delegates, delegates.length));
+                return StableUtil.renderElements(base, "Collection", reversed);


Suggested change

return StableUtil.renderElements(base, "Collection", reversed);

return StableUtil.renderElements(this, "StableList", reversed);

All other calls use "Stable...". Other view collections (ArrayList.SubList, ReverseOrderListView, AbstractMap.keySet, AbstractMap.values, HashMap.EntrySet) use the view reference (and not the underlying collection) for detecting self containment, so renderElements should use this instead of base for self.

The reason we are using "Collection" is that this method directly overrides AbstractCollection which is using "(this Collection)" for first-level circular references. I also think base should be replaced with this.

lukellmann · 2025-04-25T00:00:51Z

src/java.base/share/classes/jdk/internal/lang/stable/StableUtil.java

+            final Object value = e.getValue().wrappedContentAcquire();
+            final String valueString;
+            if (value == self) {
+                valueString = ("(this ") + selfName + ")";


Suggested change

valueString = ("(this ") + selfName + ")";

valueString = "(this " + selfName + ")";

lukellmann · 2025-04-25T00:03:31Z

src/java.base/share/classes/java/util/ImmutableCollections.java

+        @Override
+        public String toString() {
+            if (root instanceof StableList<E> stableList) {
+                return StableUtil.renderElements(root, "StableList", stableList.delegates, offset, size);


Suggested change

return StableUtil.renderElements(root, "StableList", stableList.delegates, offset, size);

return StableUtil.renderElements(this, "StableList", stableList.delegates, offset, size);

Other view collections (ArrayList.SubList, ReverseOrderListView, AbstractMap.keySet, AbstractMap.values, HashMap.EntrySet) use the view reference (and not the underlying collection) for detecting self containment, so renderElements should use this instead of root for self.

lukellmann · 2025-04-25T02:28:06Z

src/java.base/share/classes/java/util/ImmutableCollections.java

+            @Override
+            public String toString() {
+                final StableValueImpl<?>[] values = delegate.values().toArray(GENERATOR);
+                return StableUtil.renderElements(StableMap.this, "StableMap", values);


Suggested change

return StableUtil.renderElements(StableMap.this, "StableMap", values);

return StableUtil.renderElements(this, "StableCollection", values);

Other view collections (ArrayList.SubList, ReverseOrderListView, AbstractMap.keySet, AbstractMap.values, HashMap.EntrySet) use the view reference (and not the underlying collection) for detecting self containment, so renderElements should use this instead of StableMap.this for self. ImmutableCollections.StableMap.StableMapValues is a Collection, not a Map, so use "StableCollection" instead of "StableMap".

AlanBateman · 2025-04-25T06:28:59Z

src/java.base/share/classes/java/lang/StableValue.java

+     *
+     * @param other to return if the contents is not set
+     */
+    T orElse(T other);


Just to note that JEP 506 is proposing to change ScopedValue.orElse to disallow null. A ScopedValue can be bound to null (e.g. usage in Subject API) so using orElse(null) is problematic. A orNull may be added later if needed. As a StableValue can hold null then it's similar and might be surprising to developers to have the two APIs be different here. Something to look at again after JEP 502 is integrated, I'm not suggesting changing anything in this PR of course.

Yepp. Disallowing null is on the list for exploring in the next preview.

AlanBateman · 2025-04-25T10:38:14Z

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

+    // in a slow path.
+    private void preventReentry() {
+        if (Thread.holdsLock(this)) {
+            throw new IllegalStateException("Recursive initialization of a stable value is illegal");


I don't know how common it is to use holdsLocks for control flow but just to say that there are several places in the JDK that have to detect reentrancy and some of these are good candidates to use ScopedValues.

AlanBateman

I've been through the API + implementation, and a brief scan (not detailed) of the tests. Good job, it will be good to get this integrated and start to get some usage and feedback.

minborg · 2025-04-25T13:13:53Z

Thank you, @lukellmann, for your comments. Some of them were very useful! I will address them after integrating the PR.

liach

I think the design and the essential implementation meets the "95% complete" bar for preview features. There are some ceremony like toString and some other details like null values, but they should not be serious enough for this to be delivered for preview.

liach · 2025-04-25T14:41:28Z

src/java.base/share/classes/java/util/ImmutableCollections.java

+        }
+
+        @jdk.internal.ValueBased
+        final class StableMapEntrySet extends AbstractImmutableSet<Map.Entry<K, V>> {


One concern about the (non-static) inner classes is that we can't stable-annotate the immediately enclosing instance - we might need general trusting for java.util, but we need to ensure users aren't hacking collection final fields first.

JornVernee

Looks mostly good. Left a few more inline comments

JornVernee · 2025-04-29T11:22:17Z

src/java.base/share/classes/java/lang/StableValue.java

+     * @param size       the size of the allowed inputs in the continuous
+     *                   interval {@code [0, size)}


"Size of allowed inputs" doesn't seem quite right, maybe:

Suggested change

* @param size the size of the allowed inputs in the continuous

* interval {@code [0, size)}

* @param size the upper bound of the range {@code [0, size)} indicating the allowed inputs

JornVernee · 2025-04-29T11:24:36Z

src/java.base/share/classes/java/lang/StableValue.java

+     *
+     * @param size       the size of the allowed inputs in the continuous
+     *                   interval {@code [0, size)}
+     * @param underlying IntFunction used to compute cached values


Suggested change

* @param underlying IntFunction used to compute cached values

* @param underlying {@code IntFunction} used to compute cached values

JornVernee · 2025-04-29T14:51:08Z

test/jdk/java/lang/StableValue/StableFunctionTest.java

+    @MethodSource("allSets")
+    void factoryInvariants(Set<Value> inputs) {
+        assertThrows(NullPointerException.class, () -> StableValue.function(null, MAPPER));
+        assertThrows(NullPointerException.class, () -> StableValue.function(inputs, null));


I think you also need a case for null elements in inputs

There is a separate @Test void nullKeys() that takes care of that case.

JornVernee · 2025-04-29T15:07:34Z

test/jdk/java/lang/StableValue/StableValueTest.java

+                }))
+                .toList();
+        threads.forEach(Thread::start);
+        LockSupport.parkNanos(TimeUnit.MILLISECONDS.toNanos(1));


Instead of parking and calling countDown here, which might be unreliable if a particular thread doesn't get scheduled, you could set the latch value to noThreads, and then do a starter.countDown() before calling await. That should start all the threads running at the same time once all of them are ready.

JornVernee · 2025-04-29T15:19:44Z

test/micro/org/openjdk/bench/java/lang/stable/StableMethodHandleBenchmark.java

+    Object cp() {
+        CodeBuilder cob = null;
+        ConstantPoolBuilder cp = ConstantPoolBuilder.of();
+        cob.ldc(cp.constantDynamicEntry(cp.bsmEntry(cp.methodHandleEntry(BSM_CLASS_DATA), List.of()),
+                cp.nameAndTypeEntry(DEFAULT_NAME, CD_MethodHandle)));
+        return null;
+    }


Seems unused

JornVernee

The other comments I had weren't as important

minborg · 2025-04-30T16:02:13Z

/integrate

openjdk · 2025-04-30T16:03:33Z

Going to push as commit fbc4691.
Since your change was applied there have been 100 commits pushed to the master branch:

4c695fa: 8351000: StringBuilder getChar and putChar robustness
482538b: 8351884: Refactor bug8033699.java test code
0110aca: 8354791: Use Hashtable.putIfAbsent in CSS constructor
... and 97 more: https://git.openjdk.org/jdk/compare/290d24d16adcef6b9f0f5ac789b125fd7bac66a5...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-04-30T16:03:42Z

@minborg Pushed as commit fbc4691.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

liach · 2025-04-30T16:06:32Z

Congratulations! With the introduction of this as a preview feature, we can slowly improve it over time.

minborg added 30 commits August 7, 2024 16:12

Switch to acquire/release semantics

9b386d5

Switch to volatile semantics and refactor to Object

8487fbd

Remove DCL vs StableValue performance note

9e735cc

Update the happens-before section

c7dcc5c

Fix issues from comments in the PR

af2eb3c

Switch to identity hash/equals

33707c4

Improve tests

2bf5334

Fix failing test

f509352

Add additional tests

70b1a17

Clean up: visibility, formatting etc.

4658624

Add overloads for background thread resolvers

5285704

Update JEP to cover the new overloads

a5217d7

Fix error in test

b0e3b8d

Add test for recursive invocation

671dd08

Merge branch 'master' into stable-value-mini4

7cdf6ba

Update JEP

db77d00

Merge branch 'master' into stable-value-ciu

973dc33

Refactor stable value

47a1a77

Clean up and add overload

7633973

Synchronize on this

9c83228

Simplify

b5108bb

Merge branch 'master' into stable-value-ciu-small

a9ff934

Make arrays of SV trusted

ba22e20

Add support for a special CachingEnumFunction

65c83ea

Improve test

1aad10b

Require less inline space

131cfa9

Merge branch 'master' into stable-value-ciu-small

1fcc70b

Rename methods and classes

1932d07

Merge branch 'master' into stable-value-ciu-small

ee33733

Update docs and rename field

dadc3d1

minborg added 2 commits April 24, 2025 11:08

Address comments

8788713

Merge branch 'master' into implement-jep502

1740928

Revert unwanted changes

bcc022f

viktorklang-ora reviewed Apr 24, 2025

View reviewed changes

src/java.base/share/classes/java/util/ReverseOrderListView.java Show resolved Hide resolved

src/java.base/share/classes/java/util/ReverseOrderListView.java Show resolved Hide resolved

src/java.base/share/classes/java/util/ImmutableCollections.java Outdated Show resolved Hide resolved

Make public constuctor private

4839d18

viktorklang-ora approved these changes Apr 24, 2025

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated csr Pull request needs approved CSR before integration and removed ready Pull request is ready to be integrated labels Apr 24, 2025

lukellmann reviewed Apr 25, 2025

View reviewed changes

AlanBateman reviewed Apr 25, 2025

View reviewed changes

AlanBateman approved these changes Apr 25, 2025

View reviewed changes

liach approved these changes Apr 25, 2025

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Apr 29, 2025

JornVernee reviewed Apr 29, 2025

View reviewed changes

JornVernee approved these changes Apr 30, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 30, 2025

openjdk bot closed this Apr 30, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 30, 2025

lukellmann mentioned this pull request May 12, 2025

8356080: Address post-integration comments for Stable Values #25004

Closed

4 tasks

		// Used to indicate a holder value is `null` (see field `value` below)
		// A wrapper method `nullSentinel()` is used for generic type conversion.

	// attempt to modify the `wrappedValue`
	// attempt to modify `this.contents`

		return new NullableKeyValueHolder<>(k, inner.getValue().orElseSet(new Supplier<V>() {
		@Override public V get() { return mapper.apply(k); }}));

	return StableUtil.renderElements(base, "Collection", reversed);
	return StableUtil.renderElements(this, "StableList", reversed);

	valueString = ("(this ") + selfName + ")";
	valueString = "(this " + selfName + ")";

	return StableUtil.renderElements(root, "StableList", stableList.delegates, offset, size);
	return StableUtil.renderElements(this, "StableList", stableList.delegates, offset, size);

	return StableUtil.renderElements(StableMap.this, "StableMap", values);
	return StableUtil.renderElements(this, "StableCollection", values);

		* @param size the size of the allowed inputs in the continuous
		* interval {@code [0, size)}

	* @param size the size of the allowed inputs in the continuous
	* interval {@code [0, size)}
	* @param size the upper bound of the range {@code [0, size)} indicating the allowed inputs

	* @param underlying IntFunction used to compute cached values
	* @param underlying {@code IntFunction} used to compute cached values

8351565: Implement JEP 502: Stable Values (Preview) #23972

8351565: Implement JEP 502: Stable Values (Preview) #23972

Uh oh!

Conversation

minborg commented Mar 10, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Contributors

Reviewing

Uh oh!

minborg commented Apr 24, 2025

Uh oh!

openjdk bot commented Apr 24, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

viktorklang-ora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlanBateman left a comment

Choose a reason for hiding this comment

Uh oh!

minborg commented Apr 25, 2025

Uh oh!

liach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JornVernee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

minborg commented Mar 10, 2025 •

edited by openjdk bot

Loading