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

Dmitry Samersoff dms at samersoff.net
Thu Aug 22 13:48:13 UTC 2019


Frederic,

> But the ultimate decision to flatten a field or not is taken by the
> container, because it is the one which decide how the field has been
> declared, annotated, and which additional constraints the container
> has to deal with. The ValueKlass of the field does not have this
> context.

Will the separate is_flattened function within a container, not
ValueKlass, work for you?

Nevertheless, counting the fix is huge and complicated, I'm OK to leave
the code as it is.

-Dmitry

On 15.08.19 16:55, Frederic Parain wrote:
> 
> 
> On 8/12/19 8:14 AM, Dmitry Samersoff wrote:
>> 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.
> 
> I'll add it.
> 
>>
>>>> 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)
> 
> But the ultimate decision to flatten a field or not is taken by the
> container, because it is the one which decide how the field has been
> declared, annotated, and which additional constraints the container
> has to deal with. The ValueKlass of the field does not have this
> context.
> Today, having a ValueKlass with size < ValueFieldMaxFlatSize is not
> a guarantee that field will be flattened. If the field is static,
> it won't be flattened.
> Soon, if the field has been declared atomic, the decision to flatten
> won't depend on ValueFieldMaxFlatSize, but on the size of atomic
> operations supported by the platform.
> In the future, the number of parameters to consider could continue
> to increase, with consideration of alignment constraints for instance.
> All these information are in the container, so the logic to flatten
> or not must be in the computation of the layout of the container.
> 
>> Otherwise, it becomes difficult to understand what the command line
>> options (e.g. ValueFieldMaxFlatSize == 0) affect.
> 
> As explained above, ValueFieldMaxFlatSize is only one of the parameters
> considered when deciding to flatten or not.
> 
> Regards,
> 
> Fred
> 
> 
>> 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