On Mon, 12 Oct 2026 08:30:32 GMT, Johan Sjölen <[email protected]> wrote:
>> Today LayoutKind serves multiple purposes in the code:
>>
>> 1) Describe the layout of a value field [REFERENCE, *_FLAT]
>> 2) Describe the layout of a field, flat layout not applicable or flat layout
>> [UNKNOWN, *_FLAT]
>> 3) Describe the layout of a value payload [BUFFERED, *_FLAT]
>> 4) Describe uninitialized values for (1-3) [UNKNOWN]
>>
>> A surprising (to me) amount of code lands in (2).
>>
>> This leads to us having to be careful around the values that are invalid to
>> use at the various locations in the code.
>>
>> I want to make it so that it is easier to know that you are passing around
>> valid values and that we catch early if we start to use uninitialized values.
>>
>> So, I propose that we convert `LayoutKind` to only hold the flat layouts and
>> then we add a couple of wrapper classes that support the optionality of
>> holding a flat layout.
>>
>> Code that is guaranteed to operate on flat layouts will be using the
>> `FlatLayout` class.
>>
>> Code that may operate on a flat layout will be using the
>> `OptionalFlatLayout` class, which can be queried to see if it holds a
>> `FlatLayout` or not.
>>
>> `OptionalFlatLayout` adds an extra `_is_flat` boolean that is used to
>> discriminate if the `FlatLayout` is active or not. Often code already had an
>> `_is_flat` field, and for those the existing flatness field has been
>> superseded by the one in `OptionalFlatLayout`.
>>
>> Some classes already have their own, other way to decide if we are dealing
>> with a flat layout. For those I've introduced a scaled-down version, called
>> `FlatLayoutSlot`, where the existence of a `FlatLayout` is determined by the
>> surrounding code's "is flat" discriminator.
>>
>> Code that is using `OptionalFlatLayout` fields can create it to hold a flat
>> layout, create it to represent a non-flat situation, and create it marked as
>> "uninitialized". It might be that the code doesn't yet know how to
>> initialize it, or the code might only use it under certain conditions. To
>> make sure that we don't try to query uninitialized OptionalFlatLayouts,
>> asserts have been added to all layout queries.
>>
>> Other things to look for in the patch:
>>
>> * The implementation and construction of `ArrayDescription` has been
>> simplified.
>>
>> * I've decoupled the LayoutKind enumeration values from the "layout value"
>> passed around via Unsafe (and MethodHandle). See FlatLayoutEncoding.
>>
>> * Serviceability code has some restructuring around the construction of
>> objects to make sure that they can be initialized properly in the
>> initializer list.
> ...
>
> src/hotspot/share/services/heapDumper.cpp line 851:
>
>> 849:
>> 850: template<typename FieldStreamType>
>> 851: static ValueFieldInfo* calc_value_field_info(const FieldStreamType&
>> field) {
>
> Is this `calc_` prefix that informative? `value_field_info` seems fine, as
> well as `value_klass`. Or, if you're like me, you'll name them `$PREFIX_of`.
Point is: We're not really calculating anything, so seems redundant.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/33283#discussion_r4246032802
