On Fri, 12 Dec 2025 02:45:22 GMT, Quan Anh Mai <[email protected]> wrote:
>> 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?
It has never been guaranteed, nor been a goal, to have
`ValueClass::newNullRestrictedNonAtomicArray` and
`ValueClass::newNullRestrictedAtomicArray` to return the same refined array
class. The two factories aims are creating arrays with two different sets of
properties. Depending on the characteristics of the element's class, they could
have the same layout or not, but they are not the same "refinement".
Looking past JEP 401, it is likely that non-atomic flat arrays will have to
consider the length of the array in the layout selection process (because of
limitation on indexing inside humongous arrays), so it is possible that for a
given element type and a set of nullness/atomicity properties, several
refinements would exist.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1755#discussion_r2614658522
