On Mon, 8 Dec 2025 09:43:54 GMT, Tobias Hartmann <[email protected]> wrote:
>> Quan Anh Mai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Should not clone MergeMem
>
> 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.
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 {
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1755#discussion_r2611625502
