On Thu, 11 Dec 2025 18:24:46 GMT, Frederic Parain <[email protected]> wrote:
>> src/hotspot/share/prims/jvm.cpp line 493:
>>
>>> 491: } else {
>>> 492: props = ArrayKlass::ArrayProperties::NULL_RESTRICTED;
>>> 493: }
>>
>> Suggestion:
>>
>> ArrayKlass::ArrayProperties props =
>> ArrayKlass::ArrayProperties::NULL_RESTRICTED;
>> if (vk->has_non_atomic_layout()) {
>> props |= ArrayKlass::ArrayProperties::NON_ATOMIC;
>> }
>>
>>
>> Maybe double-check, if my suggestion even compiles :slightly_smiling_face:
>>
>> Regarding:
>>
>>> When creating a refined ObjArrayKlass, we fall back to ref array if the
>>> corresponding flat layout is not present, which means that if the element
>>> type is not a LooselyConsistentValue,
>>> ValueClass::newNullRestrictedNonAtomicArray will make a ref array. This
>>> seems counter-intuitive, it should have fallen back to a null-restricted
>>> atomic array instead.
>>
>> I tend to agree but this is still debatable. @fparain Thoughts?
>
> First of all, any logic deciding the layout of an array should be in
> ObjArrayKlass::array_layout_selection().
> We had layout logics in multiple places in the past, and they were getting
> out of sync very quickly.
> So let's try to keep the decision logic in a single place. (Which means that
> the jvm.cpp code has to be modified anyway to just pass NULL_RESTRICTED |
> NON_ATOMIC to the array factory).
> The fall back to the atomic layout makes sense in this case where the array
> creator asked for non-atomicity but the value class author didn't opt in
> non-atomicity.
> So the code in ObjArrayKlass::Array_layout_selection() would become:
>
>
> if (is_null_restricted(properties)) {
> if (is_non_atomic(properties)) {
> // Null-restricted + non-atomic
> if (vk->maybe_flat_in_array()) {
> if (vk->has_non_atomic_layout()) {
> return ArrayDescription(FlatArrayKlassKind, properties,
> LayoutKind::NON_ATOMIC_FLAT);
> } else if (vk->has_atomic_layout()) {
> return ArrayDescription(FlatArrayKlassKind, properties,
> LayoutKind::ATOMIC_FLAT);
> }
> }
> return ArrayDescription(RefArrayKlassKind, properties,
> LayoutKind::REFERENCE);
> } else {
@fparain Thanks for your review, This is not about choosing a layout, however,
it is about choosing a refined class for the array that is created by
`ValueClass::newNullRestrictedNonAtomicArray`. If we go with your suggestion,
then the array created by this method will have a different refined type from
an array created by `ValueClass::newNullRestrictedAtomicArray`, which seems
dangerous because C2 may try to assume that 2 classes having the same
properties should be equal. As a result, I went with this change which asserts
at `ObjArrayKlass::allocate_klass_with_properties` that non-atomic is only
requested when it is possible to do so, and changed the call sites that may
have this property popping up. The common point of those 2 sites is
`ObjArrayKlass::klass_with_properties`, so I think it is less clear whether we
should normalize the properties argument there instead. What do you think?
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1755#discussion_r2612709444
