RFR(XL): JDK-8228441 Field layout requires complete overhaul to efficiently support inline classes

[Frederic Parain]

On 8/12/19 8:14 AM, Dmitry Samersoff wrote:
> Hello Frederic,
> 
> The code looks good to me.
> 
> 
> fieldLayoutBuilder.cpp:35
> 
> this version of constructor missed size > 0 assert, I'm not sure it's
> really important but it's better to keep it uniform.

I'll add it.

> 
>>> 561, 641
>>>
>>> Does it duplicate similar code within ValueKlass, if not - could we
>>> move it there?
> 
>> I’d like to keep the two codes separated.
>> They are similar today, but we could realize that decisions to flatten
>> or not might not the the same when the container is an identity class
>> or an inline class.
> 
> 
> Sorry to disagree with you here. I would prefer to have all
> "is_flattened" logic in a single place.
> 
> I.e., if in the future this logic will differ from the default one, it
> is better to keep it inside ValueKlass with a clear path choice,
> like is_flattened(bool class_is_inline)

But the ultimate decision to flatten a field or not is taken by the
container, because it is the one which decide how the field has been
declared, annotated, and which additional constraints the container
has to deal with. The ValueKlass of the field does not have this
context.
Today, having a ValueKlass with size < ValueFieldMaxFlatSize is not
a guarantee that field will be flattened. If the field is static,
it won't be flattened.
Soon, if the field has been declared atomic, the decision to flatten
won't depend on ValueFieldMaxFlatSize, but on the size of atomic
operations supported by the platform.
In the future, the number of parameters to consider could continue
to increase, with consideration of alignment constraints for instance.
All these information are in the container, so the logic to flatten
or not must be in the computation of the layout of the container.

> Otherwise, it becomes difficult to understand what the command line
> options (e.g. ValueFieldMaxFlatSize == 0) affect.

As explained above, ValueFieldMaxFlatSize is only one of the parameters
considered when deciding to flatten or not.

Regards,

Fred


> On 07.08.19 17:27, Dmitry Samersoff wrote:
>> Hello Frederic,
>>
>>> So, I’d like to keep the old code as a backup for a little while,
>>> then remove it.
>>> If it causes to much burden for you, let me know.
>>
>> It doesn't case any burden for me, so feel free to keep this flag.
>>
>>> I’ve slightly refactored the code, and added a comment to simplify
>>> this code ever more once the old layout code is removed.
>>>
>>> I’ll include changes from your second e-mail before publishing a
>>> new webrev.
>>
>> Ok.
>>
>> This large and nice work really appreciated.
>>
>> -Dmitry
>>
>> On 06.08.19 22:47, Frederic Parain wrote:
>>> Hi Dmitry,
>>>
>>> Thank you for reviewing this huge patch!
>>>
>>>> On Aug 6, 2019, at 09:52, Dmitry Samersoff <dms at samersoff.net> wrote:
>>>>
>>>> Hello Frederic,
>>>>
>>>> General comment:
>>>>
>>>>    I'm not sure we should have and maintain UseNewLayout flag.  What is
>>>> the reason to keep old layout? Is there any code that rely on it?
>>>
>>> The hope is that the old code can be completely removed.
>>> However, this is a huge change, so it is kept as a fallback for now.
>>>
>>> Coleen has asked that the new layout code would be ported to the main line
>>> before the Valhalla integration. So I’ve also a port for JDK14 in progress
>>> (with more features, the ones not important for project Valhalla), so I’m
>>> trying to stabilize both patches at the same time.
>>> But field layout has not been changed for so long, that any change can have
>>> unexpected consequences. Some codes have very strong assumptions about layout
>>> that are in fact just accidental properties of the old code. Enabling some
>>> optimizations just breaks these codes, or reveal some bugs (I found a memory
>>> corruption in the StackWalking code, unnoticed with old layouts, breaking the
>>> StalkWaking code if the layout is slightly different).
>>>
>>> So, I’d like to keep the old code as a backup for a little while, then remove it.
>>> If it causes to much burden for you, let me know.
>>>
>>>> classFileParser.cpp:
>>>>
>>>> 5975:
>>>> ValueKlass::cast(ik)->set_first_field_offset(_first_field_offset);
>>>>
>>>> Where the value of _first_field_offset is initialized?
>>>
>>> This value, which exists only for inline classes, is initialized in
>>> FieldLayoutBuilder::compute_inline_class_layout(TRAPS):
>>>
>>>    738   _first_field_offset = _layout->start()->offset();
>>>
>>>>
>>>> heapInspection.cpp:
>>>>
>>>> 736 and below: Should we add const here?
>>>
>>> Done
>>>
>>>>
>>>> 741   AccessFlags access_flags() { return _access_flags; }
>>>>
>>>> Could we use
>>>>    const AccessFlags& access_flags() { return _access_flags; }
>>>> here?
>>>
>>> Done
>>>
>>>> 749 Please make it const char * and remove further casts
>>>
>>> Done (Sorry for the ugly code).
>>>
>>>>
>>>> 766, 775 and few locations below.
>>>>
>>>>          You may consider to create a proxy inside FieldDesc
>>>>          and replace fd.access_flags().is_static() to fd.is_static()
>>>>          for better readability.
>>>>
>>>
>>> line 766, fd is a FieldStream, not a FieldDesc, I’m not sure it worth
>>> adding the proxy.
>>> But a proxy has been added to FieldDesc for is_flattenable().
>>>
>>>
>>>> instanceKlass.cpp
>>>>
>>>> 1130 set_value_field_klass called before the check at l.1132
>>>>
>>>>      Is it correct? Should we turn the check to assert?
>>>
>>> The check is required to deal with separate compilation issues.
>>> If class C has a field of type V, V is an inline class when C
>>> is compiled, but then V is changed to be a regular class and
>>> is recompiled, the JVM has to detect the mismatch and must
>>> reject the class with an ICCE.
>>>
>>> Calling set_value_field_klass() before the check is not really
>>> an issue, if the field’s class is not an inline class, the
>>> whole class will be rejected and the stored value will never
>>> be used.
>>>
>>> The call to set_value_field_klass() could be moved after the
>>> check, but it would require to test this->get_value_field_klass_or_null(fs.index())
>>> one more time (because set_value_field_klass() must be call twice).
>>>
>>>>
>>>> valueKlass.cpp
>>>>
>>>> 52 please, add else here to improve readability.
>>>>
>>>> Could we move it to the header? Non inlined first_field_offset that just
>>>> a call to inlined get_first_field_offset looks a bit odd to me.
>>>
>>> I’ve slightly refactored the code, and added a comment to simplify
>>> this code ever more once the old layout code is removed.
>>>
>>> I’ll include changes from your second e-mail before publishing a
>>> new webrev.
>>>
>>> Thank you,
>>>
>>> Fred
>>>
>>>
>>>>
>>>>
>>>> Regards,
>>>>
>>>> -Dmitry\S
>>>>
>>>>
>>>>
>>>> On 19.07.19 18:26, Frederic Parain wrote:
>>>>> Greetings,
>>>>>
>>>>> This is the initial request for review for a new code to layout fields.
>>>>> The current code to compute field layouts is especially ugly and
>>>>> not fitted to efficiently flatten inline classes.
>>>>>
>>>>> This changeset is an almost complete overhaul of the field layout code,
>>>>> with a new framework to compute layouts and two different allocation
>>>>> strategies (one for identity classes and one for inline classes).
>>>>>
>>>>> CR: https://bugs.openjdk.java.net/browse/JDK-8228441
>>>>> Webrev: http://cr.openjdk.java.net/~fparain/layout/webrev.00/
>>>>>
>>>>> This changeset also includes some options and DCMD to ease inspection
>>>>> of layouts, especially when flattenable fields are involved.
>>>>>
>>>>> The changeset doesn’t remove the old layout code, meaning that either
>>>>> the old code or the new code can be used (controlled by VM flag).
>>>>>
>>>>> This code passed tiers 1 to 3 on Valhalla supported platforms.
>>>>> Some failures have been identified in higher tiers with some @Contended
>>>>> tests, but they are due to test bugs (tests making relying on wrong
>>>>> assumptions about field layouts).
>>>>>
>>>>> The code contains a lot of comments about the new framework and the
>>>>> different allocation strategies. Feel free to complain if comments
>>>>> are not clear enough or if some code need more comments.
>>>>>
>>>>> Regarding the huge size of this changeset, and people being in summer
>>>>> vacation, I’m not planning to push this code in the short term.
>>>>>
>>>>> Thank you,
>>>>>
>>>>> Fred
>>>>>
>>>