On Tue, 11 Mar 2025 13:22:20 GMT, Luca Kellermann <d...@openjdk.org> wrote:
>> Implement JEP 502.
>>
>> The PR passes tier1-tier3 tests.
>
> src/java.base/share/classes/java/lang/StableValue.java line 344:
>
>> 342: * {@linkplain java.lang.ref##reachability reachable} stable values
>> will hold their set
>> 343: * content perpetually.
>> 344: * <p>
>
> Should the original functions / mappers (for stable functions and
> collections) also stay reachable? Kotlin's
> [`Lazy`](https://kotlinlang.org/api/core/kotlin-stdlib/kotlin/-lazy/) [nulls
> out](https://github.com/JetBrains/kotlin/blob/c6f337283d59fcede75954eebaa589ad1b479aea/libraries/stdlib/jvm/src/kotlin/util/LazyJVM.kt#L70-L89)
> the initializer function when it's no longer needed.
The nulling out is only safe if the write to the value is visible when a
nulled-out function is visible. I think SV can ensure this, but an
implementation can easily go wrong trying to do this. (Also `orElseSet` does
not NPE if the incoming supplier is null but the value is bound)
> src/java.base/share/classes/jdk/internal/lang/stable/StableEnumFunction.java
> line 112:
>
>> 110: final Class<E> enumType =
>> (Class<E>)inputs.iterator().next().getClass();
>> 111: return (Function<T, R>) new StableEnumFunction<E, R>(enumType,
>> min, StableValueFactories.array(size), (Function<E, R>) original);
>> 112: }
>
> If `inputs` contains the enumuration constants with ordinals 0 and 2,
> wouldn't this code wrongly cause the enumeration constant with ordinal 1 to
> be an allowed input?
Indeed, a bit set predicate can be used to check input validity if it is
necessary - I think for enums, using a `StableFunction.ofEnum` dedicated API
might be better just because `StableValue` can access
`Class.getEnumConstantsShared` easily.
> src/java.base/share/classes/jdk/internal/lang/stable/StableValueImpl.java
> line 141:
>
>> 139: ? "(this StableValue)"
>> 140: : "StableValue" + renderWrapped(t);
>> 141: }
>
> Are deeper cycles of concern? I was thinking of this:
>
> var a = StableValue.of();
> var b = StableValue.of();
> a.trySet(b);
> b.trySet(a);
> System.out.println(a);
>
>
> This would solve deeper cycles for `StableValueImpl`:
>
> @Override
> public String toString() {
> final StringBuilder sb = new StringBuilder("StableValue");
> int depth = 0;
> Object t = value;
> while (t instanceof StableValueImpl<?> s) {
> if (s == this) {
> t = "(this StableValue)";
> break;
> }
> sb.append("[StableValue");
> depth++;
> t = s.value;
> }
> sb.append(renderWrapped(t));
> while (depth-- > 0) sb.append(']');
> return sb.toString();
> }
>
> This might also apply to stable functions and collections, I haven't thought
> it through for them.
I think the default Object.toString impl is better here - the type
`StableValue` shouldn't really be exposed in a user API endpoint and is just a
utility for the users. No need to bikeshed on this mostly useless functionality.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r1989965200
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r1988159371
PR Review Comment: https://git.openjdk.org/jdk/pull/23972#discussion_r1988154881
