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