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

Frederic Parain frederic.parain at oracle.com
Wed Aug 7 18:00:36 UTC 2019


Hi Dmitry,

Thank you for this detailed review.

My answers are inlined below.

> On Aug 6, 2019, at 11:50, Dmitry Samersoff <dms at samersoff.net> wrote:
> 
> 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.

A design choice was to minimize the number of dynamic allocations,
which explains why RawBlock has so many fields and pointers, to
be used in the different data structures: field lists, layouts, etc.
The counterpart of having few allocations is that RawBlock is mutated
a lot, making use of const return values not a good fit for this code.

For instance, changing next_block() to
const RawBlock* next_block()
would cause an issue with statements like this one:
b->next_block()->set_prev_block(empty);
because set_prev_block() is not an const method.

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

Constructors have been cleaned up, all default values have been removed.

> 
> fieldLayoutBuilder.cpp:
> 
> 35 This version of constructor missed assert of size.

Fixed (slightly modified handling of static fields to
avoid 0 sized RawBlocks).

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

Fixed

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

I realized this code was dead, I’ve removed it.

> 211 Debugging leftover?

As written, paranoid check :-) removed.

> 
> 469 Space missed within assert.

Fixed

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

Method is renamed (I want to keep the logic in the method to keep
the sorting methods simpler).

> 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.
For instance, flattening a field in an inline class could grow its size to the
point where the inline class itself would not be a candidate for flattening, a
concern which doesn’t exist for identity classes.

> 
> 771, 791 extra const probably not necessary here

Removed

> 
> 765, 785
> 
> Code looks very similar. Could we move it to a separate function?


Code has been refactored.

The new webrev is available here:

http://cr.openjdk.java.net/~fparain/layout/webrev.01/

The current plan is to push the code optimizing inline classes
to the Valhalla repository without waiting for the mainline
version to be finalized. After the mainline version has been
integrated, I’ll port the differences to the Valhalla repository.

Thank you,

Fred


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



More information about the valhalla-dev mailing list