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

Dmitry Samersoff dms at samersoff.net
Mon Aug 12 12:14:20 UTC 2019


Hello Frederic,

The code looks good to me.


fieldLayoutBuilder.cpp:35

this version of constructor missed size > 0 assert, I'm not sure it's
really important but it's better to keep it uniform.

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


Sorry to disagree with you here. I would prefer to have all
"is_flattened" logic in a single place.

I.e., if in the future this logic will differ from the default one, it
is better to keep it inside ValueKlass with a clear path choice,
like is_flattened(bool class_is_inline)

Otherwise, it becomes difficult to understand what the command line
options (e.g. ValueFieldMaxFlatSize == 0) affect.

-Dmitry


On 07.08.19 17:27, Dmitry Samersoff wrote:
> 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
>>>>
>>


More information about the valhalla-dev mailing list