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

[Dmitry Samersoff]

Hello Frederic,

Continuation ...

fieldLayoutBuilder.hpp:

Please review const keyword usage across this class

e.g.

80   RawBlock* prev_field() const { return _prev_field; }

probably should be

const RawBlock* prev_field() const { return _prev_field; }

There are couple of more places with the same question below.


76, 77 Default value of alignment parameter for two constructors is
different (1, -1)

77 Default value for size and alignment will trigger an assert.

Please consider refactoring of constructors. Default Initialization of
parameters with invalid values might lead to cryptic errors.

fieldLayoutBuilder.cpp:

35 This version of constructor missed assert of size.

It's better to move all parameters related code (assert and assignment)
to start of constructor.

198 It might be better to check just for EMPTY || RESERVED,
    or use a bitmask

211 Debugging leftover?

469 Space missed within assert.

This code will add new contended group for any non-existent value of g.
It's contradict with semantic assumed to get_*.

You may consider to split it to two different functions - one for the
cases when we expect new contended group and other one for cases where
we should access existing groups only. Or at least to rename it to
something like get_or_add_contended_group.

561, 641

Does it duplicate similar code within ValueKlass, if not - could we move
it there?

771, 791 extra const probably not necessary here

765, 785

Code looks very similar. Could we move it to a separate function?


Regards,
-Dmitry\S


On 06.08.19 16:52, Dmitry Samersoff 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?
> 
> 
> 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
>>