From 066477de80fc8719651b7b7bf2d02b1f58135f77 Mon Sep 17 00:00:00 2001 From: Per Minborg Date: Tue, 13 May 2025 13:40:48 +0000 Subject: [PATCH] 8356080: Address post-integration comments for Stable Values Reviewed-by: liach --- .../share/classes/java/lang/StableValue.java | 27 +-- .../java/util/ImmutableCollections.java | 224 +++++++++++++----- .../java/util/ReverseOrderListView.java | 5 +- .../lang/stable/StableEnumFunction.java | 11 +- .../internal/lang/stable/StableFunction.java | 2 +- .../lang/stable/StableIntFunction.java | 3 - .../internal/lang/stable/StableSupplier.java | 2 +- .../jdk/internal/lang/stable/StableUtil.java | 6 +- .../internal/lang/stable/StableValueImpl.java | 28 +-- .../lang/StableValue/StableFunctionTest.java | 15 +- .../java/lang/StableValue/StableListTest.java | 144 +++++++++-- .../java/lang/StableValue/StableMapTest.java | 31 +++ .../lang/StableValue/StableValueTest.java | 10 +- .../stable/StableMethodHandleBenchmark.java | 11 +- 14 files changed, 388 insertions(+), 131 deletions(-) diff --git a/src/java.base/share/classes/java/lang/StableValue.java b/src/java.base/share/classes/java/lang/StableValue.java index b12d6f5e921..1815cb1a5b1 100644 --- a/src/java.base/share/classes/java/lang/StableValue.java +++ b/src/java.base/share/classes/java/lang/StableValue.java @@ -384,8 +384,8 @@ import java.util.function.Supplier; *

* The method {@link #orElseSet(Supplier)} guarantees that the provided * {@linkplain Supplier} is invoked successfully at most once, even under race. - * 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 + * Invocations of {@link #orElseSet(Supplier)} 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 * of the same principles as {@linkplain StableValue#orElseSet(Supplier) orElseSet()} they * too are thread safe and guarantee at-most-once-per-input invocation. @@ -444,8 +444,6 @@ import java.util.function.Supplier; public sealed interface StableValue permits StableValueImpl { - // Principal methods - /** * Tries to set the contents of this StableValue to the provided {@code contents}. * The contents of this StableValue can only be set once, implying this method only @@ -509,8 +507,6 @@ public sealed interface StableValue */ T orElseSet(Supplier supplier); - // Convenience methods - /** * Sets the contents of this StableValue to the provided {@code contents}, or, if * already set, throws {@code IllegalStateException}. @@ -519,6 +515,9 @@ public sealed interface StableValue * * @param contents to set * @throws IllegalStateException if the contents was already set + * @throws IllegalStateException if a supplier invoked by {@link #orElseSet(Supplier)} + * recursively attempts to set this stable value by calling this method + * directly or indirectly. */ void setOrThrow(T contents); @@ -573,8 +572,8 @@ public sealed interface StableValue * at most once even in a multi-threaded environment. Competing threads invoking the * returned supplier's {@linkplain Supplier#get() get()} method when a value is * already under computation will block until a value is computed or an exception is - * thrown by the computing thread. The computing threads will then observe the newly - * computed value (if any) and will then never execute. + * thrown by the computing thread. The competing threads will then observe the newly + * computed value (if any) and will then never execute the {@code underlying} supplier. *

* If the provided {@code underlying} supplier throws an exception, it is rethrown * to the initial caller and no contents is recorded. @@ -614,9 +613,9 @@ public sealed interface StableValue * function for the same input, an {@linkplain IllegalStateException} will * be thrown. * - * @param size the size of the allowed inputs in the continuous - * interval {@code [0, size)} - * @param underlying IntFunction used to compute cached values + * @param size the upper bound of the range {@code [0, size)} indicating + * the allowed inputs + * @param underlying {@code IntFunction} used to compute cached values * @param the type of results delivered by the returned IntFunction * @throws IllegalArgumentException if the provided {@code size} is negative. */ @@ -684,7 +683,7 @@ public sealed interface StableValue * If invoking the provided {@code mapper} function throws an exception, it * is rethrown to the initial caller and no value for the element is recorded. *

- * Any direct {@link List#subList(int, int) subList} or {@link List#reversed()} views + * Any {@link List#subList(int, int) subList} or {@link List#reversed()} views * of the returned list are also stable. *

* The returned list and its {@link List#subList(int, int) subList} or @@ -727,8 +726,8 @@ public sealed interface StableValue * is rethrown to the initial caller and no value associated with the provided key * is recorded. *

- * Any direct {@link Map#values()} or {@link Map#entrySet()} views - * of the returned map are also stable. + * Any {@link Map#values()} or {@link Map#entrySet()} views of the returned map are + * also stable. *

* The returned map is unmodifiable and does not implement the * {@linkplain Collection##optional-operations optional operations} in the diff --git a/src/java.base/share/classes/java/util/ImmutableCollections.java b/src/java.base/share/classes/java/util/ImmutableCollections.java index 9becf167176..38cc45122a2 100644 --- a/src/java.base/share/classes/java/util/ImmutableCollections.java +++ b/src/java.base/share/classes/java/util/ImmutableCollections.java @@ -47,7 +47,6 @@ import jdk.internal.lang.stable.StableUtil; import jdk.internal.lang.stable.StableValueImpl; import jdk.internal.misc.CDS; import jdk.internal.util.ArraysSupport; -import jdk.internal.util.NullableKeyValueHolder; import jdk.internal.vm.annotation.ForceInline; import jdk.internal.vm.annotation.Stable; @@ -137,9 +136,11 @@ class ImmutableCollections { return ImmutableCollections.listFromTrustedArrayNullsAllowed(array); } public List stableList(int size, IntFunction mapper) { - return ImmutableCollections.stableList(size, mapper); + // A stable list is not Serializable, so we cannot return `List.of()` if `size == 0` + return new StableList<>(size, mapper); } public Map stableMap(Set keys, Function mapper) { + // A stable map is not Serializable, so we cannot return `Map.of()` if `keys.isEmpty()` return new StableMap<>(keys, mapper); } }); @@ -264,11 +265,6 @@ class ImmutableCollections { } } - static List stableList(int size, IntFunction mapper) { - // A lazy list is not Serializable so, we cannot return `List.of()` if size == 0 - return new StableList<>(size, mapper); - } - // ---------- List Implementations ---------- @jdk.internal.ValueBased @@ -454,17 +450,17 @@ class ImmutableCollections { } } - static final class SubList extends AbstractImmutableList + static sealed class SubList extends AbstractImmutableList implements RandomAccess { @Stable - private final AbstractImmutableList root; + final AbstractImmutableList root; @Stable - private final int offset; + final int offset; @Stable - private final int size; + final int size; private SubList(AbstractImmutableList root, int offset, int size) { assert root instanceof List12 || root instanceof ListN || root instanceof StableList; @@ -517,9 +513,8 @@ class ImmutableCollections { } } - private boolean allowNulls() { - return root instanceof ListN listN && listN.allowNulls - || root instanceof StableList; + boolean allowNulls() { + return root instanceof ListN listN && listN.allowNulls; } @Override @@ -572,14 +567,6 @@ class ImmutableCollections { return array; } - @Override - public String toString() { - if (root instanceof StableList stableList) { - return StableUtil.renderElements(root, "StableList", stableList.delegates, offset, size); - } else { - return super.toString(); - } - } } @jdk.internal.ValueBased @@ -797,8 +784,15 @@ class ImmutableCollections { } } + @FunctionalInterface + interface HasStableDelegates { + StableValueImpl[] delegates(); + } + @jdk.internal.ValueBased - static final class StableList extends AbstractImmutableList { + static final class StableList + extends AbstractImmutableList + implements HasStableDelegates { @Stable private final IntFunction mapper; @@ -879,11 +873,69 @@ class ImmutableCollections { } @Override - public String toString() { - return StableUtil.renderElements(this, "StableList", delegates); + public List subList(int fromIndex, int toIndex) { + subListRangeCheck(fromIndex, toIndex, size()); + return StableSubList.fromStableList(this, fromIndex, toIndex); } - private static final class StableReverseOrderListView extends ReverseOrderListView.Rand { + @Override + public String toString() { + return StableUtil.renderElements(this, "StableCollection", delegates); + } + + @Override + public StableValueImpl[] delegates() { + return delegates; + } + + private static final class StableSubList extends SubList + implements HasStableDelegates { + + private StableSubList(AbstractImmutableList root, int offset, int size) { + super(root, offset, size); + } + + @Override + public List reversed() { + return new StableReverseOrderListView<>(this); + } + + @Override + public List subList(int fromIndex, int toIndex) { + subListRangeCheck(fromIndex, toIndex, size()); + return StableSubList.fromStableSubList(this, fromIndex, toIndex); + } + + @Override + public String toString() { + return StableUtil.renderElements(this, "StableCollection", delegates()); + } + + @Override + boolean allowNulls() { + return true; + } + + @Override + public StableValueImpl[] delegates() { + @SuppressWarnings("unchecked") + final var rootDelegates = ((HasStableDelegates) root).delegates(); + return Arrays.copyOfRange(rootDelegates, offset, offset + size); + } + + static SubList fromStableList(StableList list, int fromIndex, int toIndex) { + return new StableSubList<>(list, fromIndex, toIndex - fromIndex); + } + + static SubList fromStableSubList(StableSubList parent, int fromIndex, int toIndex) { + return new StableSubList<>(parent.root, parent.offset + fromIndex, toIndex - fromIndex); + } + + } + + private static final class StableReverseOrderListView + extends ReverseOrderListView.Rand + implements HasStableDelegates { private StableReverseOrderListView(List base) { super(base, false); @@ -892,17 +944,23 @@ class ImmutableCollections { // This method does not evaluate the elements @Override public String toString() { - final StableValueImpl[] delegates = ((StableList)base).delegates; - final StableValueImpl[] reversed = ArraysSupport.reverse( - Arrays.copyOf(delegates, delegates.length)); - return StableUtil.renderElements(base, "Collection", reversed); + return StableUtil.renderElements(this, "StableCollection", delegates()); } @Override - public List reversed() { - return base; + public List subList(int fromIndex, int toIndex) { + final int size = base.size(); + subListRangeCheck(fromIndex, toIndex, size); + return new StableReverseOrderListView<>(base.subList(size - toIndex, size - fromIndex)); } + @Override + public StableValueImpl[] delegates() { + @SuppressWarnings("unchecked") + final var baseDelegates = ((HasStableDelegates) base).delegates(); + return ArraysSupport.reverse( + Arrays.copyOf(baseDelegates, baseDelegates.length)); + } } } @@ -1560,7 +1618,7 @@ class ImmutableCollections { @Override public boolean containsKey(Object o) { return delegate.containsKey(o); } @Override public int size() { return delegate.size(); } - @Override public Set> entrySet() { return new StableMapEntrySet(); } + @Override public Set> entrySet() { return StableMapEntrySet.of(this); } @ForceInline @Override @@ -1582,32 +1640,49 @@ class ImmutableCollections { } @jdk.internal.ValueBased - final class StableMapEntrySet extends AbstractImmutableSet> { + static final class StableMapEntrySet extends AbstractImmutableSet> { + + // Use a separate field for the outer class in order to facilitate + // a @Stable annotation. + @Stable + private final StableMap outer; @Stable private final Set>> delegateEntrySet; - StableMapEntrySet() { - this.delegateEntrySet = delegate.entrySet(); + private StableMapEntrySet(StableMap outer) { + this.outer = outer; + this.delegateEntrySet = outer.delegate.entrySet(); } - @Override public Iterator> iterator() { return new LazyMapIterator(); } + @Override public Iterator> iterator() { return LazyMapIterator.of(this); } @Override public int size() { return delegateEntrySet.size(); } - @Override public int hashCode() { return StableMap.this.hashCode(); } + @Override public int hashCode() { return outer.hashCode(); } @Override public String toString() { - return StableUtil.renderMappings(this, "StableSet", delegateEntrySet, false); + return StableUtil.renderMappings(this, "StableCollection", delegateEntrySet, false); + } + + // For @ValueBased + private static StableMapEntrySet of(StableMap outer) { + return new StableMapEntrySet<>(outer); } @jdk.internal.ValueBased - final class LazyMapIterator implements Iterator> { + static final class LazyMapIterator implements Iterator> { + + // Use a separate field for the outer class in order to facilitate + // a @Stable annotation. + @Stable + private final StableMapEntrySet outer; @Stable private final Iterator>> delegateIterator; - LazyMapIterator() { - this.delegateIterator = delegateEntrySet.iterator(); + private LazyMapIterator(StableMapEntrySet outer) { + this.outer = outer; + this.delegateIterator = outer.delegateEntrySet.iterator(); } @Override public boolean hasNext() { return delegateIterator.hasNext(); } @@ -1616,8 +1691,8 @@ class ImmutableCollections { public Entry next() { final Map.Entry> inner = delegateIterator.next(); final K k = inner.getKey(); - return new NullableKeyValueHolder<>(k, inner.getValue().orElseSet(new Supplier() { - @Override public V get() { return mapper.apply(k); }})); + return new StableEntry<>(k, inner.getValue(), new Supplier() { + @Override public V get() { return outer.outer.mapper.apply(k); }}); } @Override @@ -1627,25 +1702,60 @@ class ImmutableCollections { @Override public void accept(Entry> inner) { final K k = inner.getKey(); - action.accept(new NullableKeyValueHolder<>(k, inner.getValue().orElseSet(new Supplier() { - @Override public V get() { return mapper.apply(k); }}))); + action.accept(new StableEntry<>(k, inner.getValue(), new Supplier() { + @Override public V get() { return outer.outer.mapper.apply(k); }})); } }; delegateIterator.forEachRemaining(innerAction); } + + // For @ValueBased + private static LazyMapIterator of(StableMapEntrySet outer) { + return new LazyMapIterator<>(outer); + } + } } + private record StableEntry(K getKey, // trick + StableValueImpl stableValue, + Supplier supplier) implements Map.Entry { + + @Override public V setValue(V value) { throw uoe(); } + @Override public V getValue() { return stableValue.orElseSet(supplier); } + @Override public int hashCode() { return hash(getKey()) ^ hash(getValue()); } + @Override public String toString() { return getKey() + "=" + stableValue.toString(); } + @Override public boolean equals(Object o) { + return o instanceof Map.Entry e + && Objects.equals(getKey(), e.getKey()) + // Invoke `getValue()` as late as possible to avoid evaluation + && Objects.equals(getValue(), e.getValue()); + } + + private int hash(Object obj) { return (obj == null) ? 0 : obj.hashCode(); } + } + @Override public Collection values() { - return new StableMapValues(); + return StableMapValues.of(this); } - final class StableMapValues extends AbstractImmutableCollection { - @Override public Iterator iterator() { return new ValueIterator(); } - @Override public int size() { return StableMap.this.size(); } - @Override public boolean isEmpty() { return StableMap.this.isEmpty();} - @Override public boolean contains(Object v) { return StableMap.this.containsValue(v); } + @jdk.internal.ValueBased + static final class StableMapValues extends AbstractImmutableCollection { + + // Use a separate field for the outer class in order to facilitate + // a @Stable annotation. + @Stable + private final StableMap outer; + + private StableMapValues(StableMap outer) { + this.outer = outer; + } + + @Override public Iterator iterator() { return outer.new ValueIterator(); } + @Override public int size() { return outer.size(); } + @Override public boolean isEmpty() { return outer.isEmpty();} + @Override public boolean contains(Object v) { return outer.containsValue(v); } private static final IntFunction[]> GENERATOR = new IntFunction[]>() { @Override @@ -1656,9 +1766,15 @@ class ImmutableCollections { @Override public String toString() { - final StableValueImpl[] values = delegate.values().toArray(GENERATOR); - return StableUtil.renderElements(StableMap.this, "StableMap", values); + final StableValueImpl[] values = outer.delegate.values().toArray(GENERATOR); + return StableUtil.renderElements(this, "StableCollection", values); } + + // For @ValueBased + private static StableMapValues of(StableMap outer) { + return new StableMapValues<>(outer); + } + } @Override diff --git a/src/java.base/share/classes/java/util/ReverseOrderListView.java b/src/java.base/share/classes/java/util/ReverseOrderListView.java index 9d6d856bdb6..75d69f38aaf 100644 --- a/src/java.base/share/classes/java/util/ReverseOrderListView.java +++ b/src/java.base/share/classes/java/util/ReverseOrderListView.java @@ -33,14 +33,17 @@ import java.util.function.UnaryOperator; import java.util.stream.Stream; import java.util.stream.StreamSupport; import jdk.internal.util.ArraysSupport; +import jdk.internal.vm.annotation.Stable; /** * Provides a reverse-ordered view of a List. Not serializable. */ class ReverseOrderListView implements List { + @Stable final List base; - final boolean modifiable; + @Stable + final Boolean modifiable; public static List of(List list, boolean modifiable) { if (list instanceof RandomAccess) { diff --git a/src/java.base/share/classes/jdk/internal/lang/stable/StableEnumFunction.java b/src/java.base/share/classes/jdk/internal/lang/stable/StableEnumFunction.java index 88be2cea1b6..d6893438be5 100644 --- a/src/java.base/share/classes/jdk/internal/lang/stable/StableEnumFunction.java +++ b/src/java.base/share/classes/jdk/internal/lang/stable/StableEnumFunction.java @@ -46,7 +46,10 @@ import java.util.function.Supplier; * @implNote This implementation can be used early in the boot sequence as it does not * rely on reflection, MethodHandles, Streams etc. * + * @param enumType the class type of the Enum * @param firstOrdinal the lowest ordinal used + * @param member an int predicate that can be used to test if an enum is a member + * of the valid inputs (as there might be "holes") * @param delegates a delegate array of inputs to StableValue mappings * @param original the original Function * @param the type of the input to the function @@ -64,10 +67,8 @@ public record StableEnumFunction, R>(Class enumType, throw new IllegalArgumentException("Input not allowed: " + value); } final int index = value.ordinal() - firstOrdinal; - final StableValueImpl delegate; // Since we did the member.test above, we know the index is in bounds - delegate = delegates[index]; - return delegate.orElseSet(new Supplier() { + return delegates[index].orElseSet(new Supplier() { @Override public R get() { return original.apply(value); }}); } @@ -84,8 +85,8 @@ public record StableEnumFunction, R>(Class enumType, @Override public String toString() { + final Collection>> entries = new ArrayList<>(delegates.length); final E[] enumElements = enumType.getEnumConstants(); - final Collection>> entries = new ArrayList<>(enumElements.length); int ordinal = firstOrdinal; for (int i = 0; i < delegates.length; i++, ordinal++) { if (member.test(ordinal)) { @@ -99,7 +100,7 @@ public record StableEnumFunction, R>(Class enumType, public static , R> Function of(Set inputs, Function original) { // The input set is not empty - final Class enumType = (Class)inputs.iterator().next().getClass(); + final Class enumType = ((E) inputs.iterator().next()).getDeclaringClass(); final BitSet bitSet = new BitSet(enumType.getEnumConstants().length); int min = Integer.MAX_VALUE; int max = Integer.MIN_VALUE; diff --git a/src/java.base/share/classes/jdk/internal/lang/stable/StableFunction.java b/src/java.base/share/classes/jdk/internal/lang/stable/StableFunction.java index 1b10593e5e8..e36b4e9b25a 100644 --- a/src/java.base/share/classes/jdk/internal/lang/stable/StableFunction.java +++ b/src/java.base/share/classes/jdk/internal/lang/stable/StableFunction.java @@ -32,7 +32,7 @@ import java.util.Set; import java.util.function.Function; import java.util.function.Supplier; -// Note: It would be possible to just use `LazyMap::get` with some additional logic +// Note: It would be possible to just use `StableMap::get` with some additional logic // instead of this class but explicitly providing a class like this provides better // debug capability, exception handling, and may provide better performance. /** diff --git a/src/java.base/share/classes/jdk/internal/lang/stable/StableIntFunction.java b/src/java.base/share/classes/jdk/internal/lang/stable/StableIntFunction.java index 8bb046a762d..a921a4de87b 100644 --- a/src/java.base/share/classes/jdk/internal/lang/stable/StableIntFunction.java +++ b/src/java.base/share/classes/jdk/internal/lang/stable/StableIntFunction.java @@ -31,9 +31,6 @@ import jdk.internal.vm.annotation.Stable; import java.util.function.IntFunction; import java.util.function.Supplier; -// Note: It would be possible to just use `LazyList::get` instead of this -// class but explicitly providing a class like this provides better -// debug capability, exception handling, and may provide better performance. /** * Implementation of a stable IntFunction. *

diff --git a/src/java.base/share/classes/jdk/internal/lang/stable/StableSupplier.java b/src/java.base/share/classes/jdk/internal/lang/stable/StableSupplier.java index bdb40648db6..631a41c5710 100644 --- a/src/java.base/share/classes/jdk/internal/lang/stable/StableSupplier.java +++ b/src/java.base/share/classes/jdk/internal/lang/stable/StableSupplier.java @@ -58,7 +58,7 @@ public record StableSupplier(StableValueImpl delegate, @Override public String toString() { - final Object t = delegate.wrappedContentAcquire(); + final Object t = delegate.wrappedContentsAcquire(); return t == this ? "(this StableSupplier)" : StableValueImpl.renderWrapped(t); } diff --git a/src/java.base/share/classes/jdk/internal/lang/stable/StableUtil.java b/src/java.base/share/classes/jdk/internal/lang/stable/StableUtil.java index f6f33f9b1e8..74104ddbb49 100644 --- a/src/java.base/share/classes/jdk/internal/lang/stable/StableUtil.java +++ b/src/java.base/share/classes/jdk/internal/lang/stable/StableUtil.java @@ -47,7 +47,7 @@ public final class StableUtil { int length) { final StringJoiner sj = new StringJoiner(", ", "[", "]"); for (int i = 0; i < length; i++) { - final Object value = delegates[i + offset].wrappedContentAcquire(); + final Object value = delegates[i + offset].wrappedContentsAcquire(); if (value == self) { sj.add("(this " + selfName + ")"); } else { @@ -63,10 +63,10 @@ public final class StableUtil { boolean curly) { final StringJoiner sj = new StringJoiner(", ", curly ? "{" : "[", curly ? "}" : "]"); for (var e : delegates) { - final Object value = e.getValue().wrappedContentAcquire(); + final Object value = e.getValue().wrappedContentsAcquire(); final String valueString; if (value == self) { - valueString = ("(this ") + selfName + ")"; + valueString = "(this " + selfName + ")"; } else { valueString = StableValueImpl.renderWrapped(value); } diff --git a/src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java b/src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java index 88c80eb6b39..1413fd7446e 100644 --- a/src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java +++ b/src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java @@ -51,10 +51,10 @@ public final class StableValueImpl implements StableValue { // Unsafe offsets for direct field access - private static final long CONTENT_OFFSET = + private static final long CONTENTS_OFFSET = UNSAFE.objectFieldOffset(StableValueImpl.class, "contents"); - // 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) private static final Object NULL_SENTINEL = new Object(); // Generally, fields annotated with `@Stable` are accessed by the JVM using special @@ -77,13 +77,13 @@ public final class StableValueImpl implements StableValue { @ForceInline @Override public boolean trySet(T contents) { - if (wrappedContentAcquire() != null) { + if (wrappedContentsAcquire() != null) { return false; } // Prevent reentry via an orElseSet(supplier) preventReentry(); // Mutual exclusion is required here as `orElseSet` might also - // attempt to modify the `wrappedValue` + // attempt to modify `this.contents` synchronized (this) { return wrapAndSet(contents); } @@ -102,7 +102,7 @@ public final class StableValueImpl implements StableValue { @ForceInline @Override public T orElseThrow() { - final Object t = wrappedContentAcquire(); + final Object t = wrappedContentsAcquire(); if (t == null) { throw new NoSuchElementException("No contents set"); } @@ -112,21 +112,21 @@ public final class StableValueImpl implements StableValue { @ForceInline @Override public T orElse(T other) { - final Object t = wrappedContentAcquire(); + final Object t = wrappedContentsAcquire(); return (t == null) ? other : unwrap(t); } @ForceInline @Override public boolean isSet() { - return wrappedContentAcquire() != null; + return wrappedContentsAcquire() != null; } @ForceInline @Override public T orElseSet(Supplier supplier) { Objects.requireNonNull(supplier); - final Object t = wrappedContentAcquire(); + final Object t = wrappedContentsAcquire(); return (t == null) ? orElseSetSlowPath(supplier) : unwrap(t); } @@ -149,7 +149,7 @@ public final class StableValueImpl implements StableValue { @Override public String toString() { - final Object t = wrappedContentAcquire(); + final Object t = wrappedContentsAcquire(); return t == this ? "(this StableValue)" : renderWrapped(t); @@ -158,8 +158,8 @@ public final class StableValueImpl implements StableValue { // Internal methods shared with other internal classes @ForceInline - public Object wrappedContentAcquire() { - return UNSAFE.getReferenceAcquire(this, CONTENT_OFFSET); + public Object wrappedContentsAcquire() { + return UNSAFE.getReferenceAcquire(this, CONTENTS_OFFSET); } static String renderWrapped(Object t) { @@ -185,11 +185,11 @@ public final class StableValueImpl implements StableValue { * @return if the contents was set */ @ForceInline - private boolean wrapAndSet(Object newValue) { + private boolean wrapAndSet(T newValue) { assert Thread.holdsLock(this); // We know we hold the monitor here so plain semantic is enough if (contents == null) { - UNSAFE.putReferenceRelease(this, CONTENT_OFFSET, wrap(newValue)); + UNSAFE.putReferenceRelease(this, CONTENTS_OFFSET, wrap(newValue)); return true; } return false; diff --git a/test/jdk/java/lang/StableValue/StableFunctionTest.java b/test/jdk/java/lang/StableValue/StableFunctionTest.java index 4476c046957..07f51470a0b 100644 --- a/test/jdk/java/lang/StableValue/StableFunctionTest.java +++ b/test/jdk/java/lang/StableValue/StableFunctionTest.java @@ -52,7 +52,13 @@ final class StableFunctionTest { ZERO(0), ILLEGAL_BEFORE(-1), // Valid values - THIRTEEN(13), + THIRTEEN(13) { + @Override + public String toString() { + // getEnumConstants will be `null` for this enum as it is overridden + return super.toString()+" (Overridden)"; + } + }, ILLEGAL_BETWEEN(-2), FORTY_TWO(42), // Illegal values (not in the input set) @@ -196,6 +202,13 @@ final class StableFunctionTest { assertEquals("jdk.internal.lang.stable.StableFunction", emptyFunction.getClass().getName()); } + @Test + void overriddenEnum() { + final var overridden = Value.THIRTEEN; + Function enumFunction = StableValue.function(EnumSet.of(overridden), Value::asInt); + assertEquals(MAPPER.apply(overridden), enumFunction.apply(overridden)); + } + private static Stream> nonEmptySets() { return Stream.of( Set.of(Value.FORTY_TWO, Value.THIRTEEN), diff --git a/test/jdk/java/lang/StableValue/StableListTest.java b/test/jdk/java/lang/StableValue/StableListTest.java index f0ae081c76c..2abe305b0e7 100644 --- a/test/jdk/java/lang/StableValue/StableListTest.java +++ b/test/jdk/java/lang/StableValue/StableListTest.java @@ -35,7 +35,6 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.MethodSource; import java.io.Serializable; -import java.util.Arrays; import java.util.Comparator; import java.util.IdentityHashMap; import java.util.List; @@ -46,7 +45,9 @@ import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; +import java.util.function.Function; import java.util.function.IntFunction; +import java.util.function.UnaryOperator; import java.util.stream.Collectors; import java.util.stream.IntStream; import java.util.stream.Stream; @@ -242,18 +243,8 @@ final class StableListTest { assertEquals(eq.toString(), lazy.toString()); } - @Test - void subListToString() { - subListToString0(newList()); - subListToString0(newList().subList(1, SIZE)); - subListToString0(newList().subList(1, SIZE).subList(0, SIZE - 2)); - } - - void subListToString0(List subList) { + void assertUnevaluated(List subList) { assertEquals(asString(".unset", subList), subList.toString()); - - var first = subList.getFirst(); - assertEquals(asString(first.toString(), subList), subList.toString()); } @Test @@ -272,17 +263,47 @@ final class StableListTest { } @Test - void reversedToString() { - var reversed = newList().reversed(); - subListToString0(reversed); + void sublistReversedToString() { + var actual = StableValue.list(4, IDENTITY); + var expected = List.of(0, 1, 2, 3); + for (UnaryOperation op : List.of( + new UnaryOperation("subList", l -> l.subList(1, 3)), + new UnaryOperation("reversed", List::reversed))) { + actual = op.apply(actual); + expected = op.apply(expected); + } + // Touch one of the elements + actual.getLast(); + + var actualToString = actual.toString(); + var expectedToString = expected.toString().replace("2", ".unset"); + assertEquals(expectedToString, actualToString); } + // This test makes sure successive view operations retains the property + // of being a Stable view. @Test - void subListReversedToString() { - var list = newList().subList(1, SIZE - 1).reversed(); - // This combination is not lazy. There has to be a limit somewhere. - var regularList = newRegularList().subList(1, SIZE - 1).reversed(); - assertEquals(regularList.toString(), list.toString()); + void viewsStable() { + viewOperations().forEach(op0 -> { + viewOperations().forEach( op1 -> { + viewOperations().forEach(op2 -> { + var list = newList(); + var view1 = op0.apply(list); + var view2 = op1.apply(view1); + var view3 = op2.apply(view2); + var className3 = className(view3); + var transitions = className(list) + ", " + + op0 + " -> " + className(view1) + ", " + + op1 + " -> " + className(view2) + ", " + + op2 + " -> " + className3; + assertTrue(className3.contains("Stable"), transitions); + assertUnevaluated(list); + assertUnevaluated(view1); + assertUnevaluated(view2); + assertUnevaluated(view3); + }); + }); + }); } @Test @@ -294,6 +315,23 @@ final class StableListTest { assertEquals("Recursive initialization of a stable value is illegal", x.getMessage()); } + @Test + void indexOfNullInViews() { + final int size = 5; + final int middle = 2; + viewOperations().forEach(op0 -> { + viewOperations().forEach( op1 -> { + viewOperations().forEach(op2 -> { + var list = StableValue.list(size, x -> x == middle ? null : x);; + var view1 = op0.apply(list); + var view2 = op1.apply(view1); + var view3 = op2.apply(view2); + assertEquals(middle, view3.indexOf(null)); + }); + }); + }); + } + // Immutability @ParameterizedTest @@ -362,6 +400,39 @@ final class StableListTest { assertEquals(SIZE, idMap.size()); } + @Test + void childObjectOpsLazy() { + viewOperations().forEach(op0 -> { + viewOperations().forEach(op1 -> { + viewOperations().forEach(op2 -> { + childOperations().forEach(co -> { + var list = newList(); + var view1 = op0.apply(list); + var view2 = op1.apply(view1); + var view3 = op2.apply(view2); + var child = co.apply(view3); + var childClassName = className(child); + var transitions = className(list) + ", " + + op0 + " -> " + className(view1) + ", " + + op1 + " -> " + className(view2) + ", " + + op2 + " -> " + className(view3) + ", " + + co + " -> " + childClassName; + + // None of these operations should trigger evaluation + var childToString = child.toString(); + int childHashCode = child.hashCode(); + boolean childEqualToNewObj = child.equals(new Object()); + + assertUnevaluated(list); + assertUnevaluated(view1); + assertUnevaluated(view2); + assertUnevaluated(view3); + }); + }); + }); + }); + } + // Support constructs record Operation(String name, @@ -370,6 +441,36 @@ final class StableListTest { @Override public String toString() { return name; } } + record UnaryOperation(String name, + UnaryOperator> operator) implements UnaryOperator> { + @Override public List apply(List list) { return operator.apply(list); } + @Override public String toString() { return name; } + } + + record ListFunction(String name, + Function, Object> function) implements Function, Object> { + @Override public Object apply(List list) { return function.apply(list); } + @Override public String toString() { return name; } + } + + static Stream viewOperations() { + return Stream.of( + // We need identity to capture all combinations + new UnaryOperation("identity", l -> l), + new UnaryOperation("reversed", List::reversed), + new UnaryOperation("subList", l -> l.subList(0, l.size())) + ); + } + + static Stream childOperations() { + return Stream.of( + // We need identity to capture all combinations + new ListFunction("iterator", List::iterator), + new ListFunction("listIterator", List::listIterator), + new ListFunction("listIterator", List::stream) + ); + } + static Stream nullAverseOperations() { return Stream.of( new Operation("forEach", l -> l.forEach(null)), @@ -433,4 +534,7 @@ final class StableListTest { .collect(Collectors.joining(", ")) + "]"; } + static String className(Object o) { + return o.getClass().getName(); + } } diff --git a/test/jdk/java/lang/StableValue/StableMapTest.java b/test/jdk/java/lang/StableValue/StableMapTest.java index 3133acc2f29..86cf4ab3643 100644 --- a/test/jdk/java/lang/StableValue/StableMapTest.java +++ b/test/jdk/java/lang/StableValue/StableMapTest.java @@ -248,6 +248,37 @@ final class StableMapTest { assertEquals(KEYS, encountered); } + @Test + void stableEntry() { + var map = newMap(); + var entry = map.entrySet().stream() + .filter(e -> e.getKey().equals(KEY)) + .findAny() + .orElseThrow(); + + assertEquals(KEY + "=.unset", entry.toString()); + var otherDifferent = Map.entry(-1, -1); + assertNotEquals(entry, otherDifferent); + assertEquals(KEY + "=.unset", entry.toString()); + var otherEqual = Map.entry(KEY, KEY); + assertEquals(entry, otherEqual); + assertEquals(KEY + "=" + KEY, entry.toString()); + assertEquals(entry.hashCode(), otherEqual.hashCode()); + } + + @Test + void stableForEachEntry() { + var map = newMap(); + // Only touch the key. + map.entrySet().iterator().forEachRemaining(Map.Entry::getKey); + map.entrySet().iterator() + .forEachRemaining(e -> assertTrue(e.toString().contains(".unset"))); + // Only touch the value. + map.entrySet().iterator().forEachRemaining(Map.Entry::getValue); + map.entrySet().iterator() + .forEachRemaining(e -> assertFalse(e.toString().contains(".unset"))); + } + // Immutability @ParameterizedTest @MethodSource("unsupportedOperations") diff --git a/test/jdk/java/lang/StableValue/StableValueTest.java b/test/jdk/java/lang/StableValue/StableValueTest.java index 0ed68352509..4290c8716a0 100644 --- a/test/jdk/java/lang/StableValue/StableValueTest.java +++ b/test/jdk/java/lang/StableValue/StableValueTest.java @@ -29,6 +29,7 @@ import org.junit.jupiter.api.Test; +import java.util.ArrayList; import java.util.List; import java.util.Map; import java.util.NoSuchElementException; @@ -355,12 +356,14 @@ final class StableValueTest { void race(BiPredicate, Integer> winnerPredicate) { int noThreads = 10; - CountDownLatch starter = new CountDownLatch(1); + CountDownLatch starter = new CountDownLatch(noThreads); StableValue stable = StableValue.of(); Map winners = new ConcurrentHashMap<>(); List threads = IntStream.range(0, noThreads).mapToObj(i -> new Thread(() -> { try { - // Ready, set ... + // Ready ... + starter.countDown(); + // ... set ... starter.await(); // Here we go! winners.put(i, winnerPredicate.test(stable, i)); @@ -370,9 +373,6 @@ final class StableValueTest { })) .toList(); threads.forEach(Thread::start); - LockSupport.parkNanos(TimeUnit.MILLISECONDS.toNanos(1)); - // Start the race - starter.countDown(); threads.forEach(StableValueTest::join); // There can only be one winner assertEquals(1, winners.values().stream().filter(b -> b).count()); diff --git a/test/micro/org/openjdk/bench/java/lang/stable/StableMethodHandleBenchmark.java b/test/micro/org/openjdk/bench/java/lang/stable/StableMethodHandleBenchmark.java index 5017fb4e11a..95721d88cec 100644 --- a/test/micro/org/openjdk/bench/java/lang/stable/StableMethodHandleBenchmark.java +++ b/test/micro/org/openjdk/bench/java/lang/stable/StableMethodHandleBenchmark.java @@ -65,7 +65,8 @@ public class StableMethodHandleBenchmark { private static final MethodHandle FINAL_MH = identityHandle(); private static final StableValue STABLE_MH; - private static MethodHandle mh = identityHandle(); + + private static /* intentionally not final */ MethodHandle mh = identityHandle(); private static final Dcl DCL = new Dcl<>(StableMethodHandleBenchmark::identityHandle); private static final AtomicReference ATOMIC_REFERENCE = new AtomicReference<>(identityHandle()); private static final Map MAP = new ConcurrentHashMap<>(); @@ -112,14 +113,6 @@ public class StableMethodHandleBenchmark { return (int) STABLE_MH.orElseThrow().invokeExact(1); } - 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; - } - static MethodHandle identityHandle() { var lookup = MethodHandles.lookup(); try {