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

[Dmitry Samersoff]

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?


classFileParser.cpp:

5975:
ValueKlass::cast(ik)->set_first_field_offset(_first_field_offset);

Where the value of _first_field_offset is initialized?

heapInspection.cpp:

736 and below: Should we add const here?

741   AccessFlags access_flags() { return _access_flags; }

Could we use
   const AccessFlags& access_flags() { return _access_flags; }
here?

749 Please make it const char * and remove further casts


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.

instanceKlass.cpp

1130 set_value_field_klass called before the check at l.1132

     Is it correct? Should we turn the check to assert?

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.


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
>