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

[Dmitry Samersoff]

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
>>>
>