[External] : RE: Question on the inline type flattening decision

[Xiaohong Gong]

Hi Fred,

Thanks for the updating!

> Field flattening has two major side effects: atomicity and size.
> 
> Final fields are not subject to atomicity issues because they are immutable after their initialization.
> 
> Both final and non-final fields have an impact on the object size, and potentially on cache behavior.
> Bigger objects are less likely to fit in data caches, and bigger distances between fields would require more cache lines and more cache misses to read them. This issue is not significant when accessing fields of a single object, but it can become dominant when accessing fields of objects stored in a flat array.
> 
> So, in theory, the flattening test should be:
> 
>    if (!((!fieldinfo.access_flags().is_final() && (too_atomic_to_flatten 
> || too_volatile_to_flatten))
>          || too_big_to_flatten)) {
> 
> Atomicity constraints are considered only for non-final fields, and size constraints are considered for all fields.

This sounds reasonable to me. So final fields with larger size is not better to be flattened. Thanks a lot! I will fix it in few days.

Best Regards,
Xiaohong

-----Original Message-----
From: Frederic Parain <frederic.parain at oracle.com> 
Sent: Monday, July 10, 2023 11:09 PM
To: Xiaohong Gong <Xiaohong.Gong at arm.com>; valhalla-dev at openjdk.org
Cc: nd <nd at arm.com>
Subject: Re: [External] : RE: Question on the inline type flattening decision

Hi Xiaohong,


Field flattening has two major side effects: atomicity and size.

Final fields are not subject to atomicity issues because they are immutable after their initialization.

Both final and non-final fields have an impact on the object size, and potentially on cache behavior.
Bigger objects are less likely to fit in data caches, and bigger distances between fields would require more cache lines and more cache misses to read them. This issue is not significant when accessing fields of a single object, but it can become dominant when accessing fields of objects stored in a flat array.

So, in theory, the flattening test should be:

   if (!((!fieldinfo.access_flags().is_final() && (too_atomic_to_flatten 
|| too_volatile_to_flatten))
         || too_big_to_flatten)) {

Atomicity constraints are considered only for non-final fields, and size constraints are considered for all fields.


That being said, we have always been more aggressive in the flattening 
of final fields because it was
beneficial to C2.

At this point, I don't think I have the data to say which flattening 
policy is better.

Best Regards,


Fred


On 7/6/23 9:53 PM, Xiaohong Gong wrote:
> Hi Fred,
>
>> I would recommend to modify the test driving the flattening decisions the following way:
>>
>> if (InlineFieldMaxFlatSize != 0
>>
>>        && (!(too_big_to_flatten | too_atomic_to_flatten |
>> too_volatile_to_flatten) || fieldinfo.access_flags().is_final()) {
>>
>>
>> By adding the first term "InlineFieldMaxFlatSize != 0 ", it will be
>> obvious that this is the way to completely disable field flattening.
>>
>> The second term would remain the more specific tests on the field's
>> properties to decide to flatten or not when flattening is enabled.
>>
>>
>> What do you think of this proposal?
> Thanks for the proposal! But this looks more like a workaround for me. Actually this can
> disable the field flattening with "-XX:InlineFieldMaxFlatSize=0". But what if setting it to
> other small value like "2" ? That is, the field can be flattened if its holder class size is larger
> than the specified max flat size, isn't it?
>
> Thanks,
> Xiaohong
>
> -----Original Message-----
> From: Frederic Parain <frederic.parain at oracle.com>
> Sent: Thursday, July 6, 2023 8:57 PM
> To: Xiaohong Gong <Xiaohong.Gong at arm.com>; valhalla-dev at openjdk.org
> Cc: nd <nd at arm.com>
> Subject: Re: [External] : RE: Question on the inline type flattening decision
>
> Hi Xiaohong,
>
>
> I would recommend to modify the test driving the flattening decisions the following way:
>
> if (InlineFieldMaxFlatSize != 0
>
>        && (!(too_big_to_flatten | too_atomic_to_flatten |
> too_volatile_to_flatten) || fieldinfo.access_flags().is_final()) {
>
>
> By adding the first term "InlineFieldMaxFlatSize != 0 ", it will be
> obvious that this is the way to completely disable field flattening.
>
> The second term would remain the more specific tests on the field's
> properties to decide to flatten or not when flattening is enabled.
>
>
> What do you think of this proposal?
>
>
> Best Regards,
>
>
> Fred
>
>
> On 7/2/23 9:23 PM, Xiaohong Gong wrote:
>> Hi Frederic,
>>
>> Thanks for looking at this issue. Yes, I was planning to fix this. But I'm not sure how to handle the relationship with "final" fields.
>>
>> One is removing the followed "field" access check, which means the fields can be flattened no matter whether the field is "final" or not.
>>
>> Another is changing the followed "||" to "&&", so that only the "final" fields that match the preconditions can be flattened.
>>
>> So which one do you think is the defined behavior? The first one sounds reasonable to me. WDYT? Thanks a lot!
>>
>> BTW, I'v filed this issue to: https://bugs.openjdk.org/browse/JDK-8311219
>>
>> Best Regards,
>> Xiaohong
>>
>> -----Original Message-----
>> From: Frederic Parain <frederic.parain at oracle.com>
>> Sent: Friday, June 30, 2023 9:28 PM
>> To: Xiaohong Gong <Xiaohong.Gong at arm.com>; valhalla-dev at openjdk.org
>> Cc: nd <nd at arm.com>
>> Subject: Re: Question on the inline type flattening decision
>>
>> Hi Xiaohong,
>>
>>
>> Thank you for reporting this. It looks like a bug to me, specifying
>> -XX:InlineFieldMaxFlatSize=0 should disable all field flattening, including final fields.
>>
>> Do you want to fill a bug report or do you want me to take care of it?
>>
>>
>> Best Regards,
>>
>>
>> Fred
>>
>>
>> On 6/30/23 3:04 AM, Xiaohong Gong wrote:
>>> Hi,
>>>
>>> I guess this is the right place to ask this question related to the
>>> flattening decision on inline type fields?
>>>
>>> I met an issue when I was running the tests under
>>> “hotspot/jtreg/compiler/valhalla/inlinetypes” with
>>> “-XX:InlineFieldMaxFlatSize=0”. The intention is doing some testing by
>>> forcing the inline type fields not be flattened.  And I debugged some
>>> C2 code in `inlinetypenode.cpp` like `InlineTypeNode::load()`.  But I
>>> found that the code path has no difference, which means this flag
>>> cannot work and the field is flattened as without this flag.
>>>
>>> And then, I checked the relative code
>>> https://urldefense.com/v3/__https://github.com/openjdk/valhalla/blob/lworld/src/hotspot/share/clas__;!!ACWV5N9M2RV99hQ!J7daZI_Hay5kWckchOY_VRTIwQvkJzF8AtUA0SEbDccLvtWEIcMKpkQY4IFKruqGd1K0_qzZXVvoPrHk3ys7z-iq8Q$
>>> sfile/fieldLayoutBuilder.cpp#L759 to find out the reason. It checks
>>> the three necessary conditions (i.e.
>>> “the code size comparison with `InlineFieldMaxSize`”, “atomic” or
>>> “volatile” fields) firstly, which is the right behavior I think. But
>>> then the result is “||” with the `final` access flag. Which means if
>>> the field is `final`, it will be flattened anyway regardless of the
>>> necessary three limitations.
>>>
>>> So is this the expected behavior? Does this mean the inline type field
>>> can always be flattened if it is declared with `final`? But I didn’t
>>> find any descriptions on the flattening decision related to the
>>> `final` flag.
>>>
>>> Many thanks if any help on this!
>>>
>>> Best Regards,
>>>
>>> Xiaohong
>>>