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

Frederic Parain frederic.parain at oracle.com
Thu Aug 22 14:14:26 UTC 2019



> On Aug 22, 2019, at 09:48, Dmitry Samersoff <dms at samersoff.net> wrote:
> 
> 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?

The current approach is to encapsulate all the field layout logic
inside the FieldLayoutBuilder class, so there’s a single place
to look at for all layout decisions, and this logic is executed
in the context of the container. I’d like to keep it this
way unless we find a very good rational to delegate part of this
logic to another VM component.

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

Thank you for being flexible and for the review.
As stated above, we might reconsider the choice of the is_flattened()
method as the project evolves.

Best Regards,

Fred

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