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

[Frederic Parain]

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