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