On Wed, 17 Apr 2024 14:07:05 GMT, Chen Liang <li...@openjdk.org> wrote:
> Question:
>
> 1. Will we ever try to expose the stable benign race model to users?
> 2. Will we ever try to inline the stable values in object layout like a
> stable field?
1. I think there is little or no upside in exposing the benign race. It would
also be difficult to assert the invariant, competing objects are functionally
equivalent. So, I think no.
2. In a static context, the stable value will be inlined (or rather
constant-folded). So we are partly already there. For pure instance contexts, I
have some ideas about this but it is non-trivial.
> Just curious, can you test other samples, like `StableValue<List<Object>>`
> where the contained `List` is an immutable list from `List.of` factories? I
> think that would be a meaningful case too.
Good suggestion. I've added such a test. It turns out the performance is great
there too.
> Also on a side note, I just realized there's no equivalent of `@Stable int[]`
> etc. stable primitive arrays exposed, yet immutable arrays will be useful. Is
> the Frozen Arrays JEP still active, or will this Stable Values consider
> expose stable primitive arrays?
Good question. In one of the previous prototypes, we accepted a class literal
that would enable the use of primitive arrays. We now think that we can achieve
the same thing once Valhalla is integrated. This will allow not just
`StableValue` to use primitive flattened arrays but also a large number of
other constructs like `ArrayList`.
One thing we are considering is adding support for stable multi-dimensional
reference arrays. For an overwhelming majority of the use cases, we would be
able to eliminate the second layer of indirection that is there for arrays of
rank > 1.
> src/java.base/share/classes/java/lang/reflect/Field.java line 179:
>
>> 177: AccessibleObject.checkPermission();
>> 178: // Always check if the field is a final StableValue
>> 179: if (StableValue.class.isAssignableFrom(type) &&
>> Modifier.isFinal(modifiers)) {
>
> This doesn't protect the Stable Collections.
I will take a look at having an interface that signals this.
> src/java.base/share/classes/java/util/ImmutableCollections.java line 173:
>
>> 171: .map(Objects::requireNonNull)
>> 172: .toArray();
>> 173: return keys instanceof EnumSet
>
> We can move this instanceof check before the stream call.
As we need the array in both cases, how would such a solution look like without
duplicating code?
> src/java.base/share/classes/java/util/ImmutableCollections.java line 1457:
>
>> 1455: private final V[] elements;
>> 1456: @Stable
>> 1457: private final AuxiliaryArrays aux;
>
> Is java.util not trusted package so we need `@Stable`?
That is correct. Hence, there are many @Stable annotations already in this
class.
> src/java.base/share/classes/java/util/ImmutableCollections.java line 1677:
>
>> 1675: static final class StableEnumMap<K extends Enum<K>, V>
>> 1676: extends AbstractImmutableMap<K, StableValue<V>>
>> 1677: implements Map<K, StableValue<V>>, HasComputeIfUnset<K, V>
>> {
>
> Note that this might be a navigable map, as enums are comparable.
While that is true, no other immutable collection implements a navigable map.
The way the API is currently wired, it always returns a `Map`. If we go down
this route, it would incidentally return a `NaviableMap` if presented with an
`EnumMap` or, we could have a separate factory for enums that states it returns
a `NavigableMap`. I think creating all the required views would increase
complexity significantly and I am not sure it would be used that much. That
said, let us keep this open for the future.
> src/java.base/share/classes/java/util/ImmutableCollections.java line 1855:
>
>> 1853: @Override
>> 1854: public boolean equals(Object o) {
>> 1855: return o == this;
>
> These implementations are violations to the Set contracts; Set's hash code
> should be its elements' sum (thus an entry set's hash code is equivalent to
> its map's hash) and equals should check if all elements are present. This
> also makes two entry sets from two `entrySet()` calls not equal (at least
> before valhalla)
Good catch. Thank you for finding this!
> src/java.base/share/classes/jdk/internal/lang/StableValue.java line 279:
>
>> 277: static <V> V computeIfUnset(List<StableValue<V>> list,
>> 278: int index,
>> 279: IntFunction<? extends V> mapper) {
>
> Hmm, these APIs seem unintuitive and error-prone to users. Have you studied
> the use case where for one stable list/map, there are distinct initialization
> logics for different indices/keys so that they support different mappers for
> the same list/map? I cannot recall on top of my head.
>
> If we drop said ability and restrict mappers to the list/map creation, the
> whole thing will be much cleaner, and it's a better way to avoid capturing
> lambdas as well. Users can still go to individual stable values and use
> functional creation if they really, really want that functionality.
I see what you mean with distinct initialization logic. This is not the
intended use though.
The reason these methods exist is to avoid lambda capturing. Let's say we have
a `Function<K, V>` we want to apply to a `Map<K, StableValue<V>>`. Then,
retrieving a `stable = StableValue<V>` and applying `stable.computeIfUnset(()
-> function.apply(key))` would capture a new `Supplier`. Another alternative
would be to write imperative code similar to what is already in these methods.
What we could do is provide factories for memorized functions (the latter
described in the draft JEP at the end (https://openjdk.org/jeps/8312611) ) even
though these are easy to write.
I think what you are proposing is something like this?
Map<K, StableValue<V>> map = StableValue.ofMap(keys, k -> createV(k));
or perhaps even:
Map<K, V> map = StableValue.ofMap(keys, k -> createV(k));
-------------
PR Comment: https://git.openjdk.org/jdk/pull/18794#issuecomment-2061521478
PR Comment: https://git.openjdk.org/jdk/pull/18794#issuecomment-2076524102
PR Comment: https://git.openjdk.org/jdk/pull/18794#issuecomment-2076528648
PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1591463541
PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1598111344
PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1599938827
PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1568366235
PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1599960795
PR Review Comment: https://git.openjdk.org/jdk/pull/18794#discussion_r1568387185
