[External] : RE: Question on the inline type flattening decision
Bhateja, Jatin
jatin.bhateja at intel.com
Tue Jul 11 10:00:09 UTC 2023
> -----Original Message-----
> From: valhalla-dev <valhalla-dev-retn at openjdk.org> On Behalf Of Xiaohong
> Gong
> Sent: Tuesday, July 11, 2023 12:46 PM
> To: Frederic Parain <frederic.parain at oracle.com>; valhalla-dev at openjdk.org
> Cc: nd <nd at arm.com>
> Subject: RE: [External] : RE: Question on the inline type flattening decision
>
> 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.
Few more observations on this:-
- Flattening a value field of an identity object appears space efficient since we may be able to save on header bits (exact_size vs instance_size).
- In the long run, aggressively in-lining value fields of an identity object may have caching penalties (as Fred also pointed out) and we may also miss out on interning opportunities given that values are immutable.
I also discovered that currently we are not folding the logic around truly constant field access, in the following case I expected most of the field access w.r.t to root to get folded by compiler.
However, if we make large_inline_fields_holder a value class then constant folding gets enforced, ideally from a usage standpoint a method taking all constant value argument should be closer to
c++11 constexpr semantics.
value class payload {
long f1;
long f2;
public payload(long f1, long f2) {
this.f1 = f1;
this.f2 = f2;
}
}
public class large_inline_fields_holder {
public payload lf1;
public payload lf2;
public static final large_inline_fields_holder root = new large_inline_fields_holder(10);
public large_inline_fields_holder(long val) {
lf1 = new payload(val , val + 10);
lf2 = new payload(val + 20 , val + 30);
}
public static long micro (int i) {
return root.lf1.f1 + root.lf1.f2 + root.lf2.f1 + root.lf2.f2 + i;
}
public static void main(String [] args) {
long res = 0;
for (int i = 0; i < 10000; i++) {
res += micro(i);
}
System.out.println(res);
}
}
>
> 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_Hay5kWc
> kch
> >>>
> OY_VRTIwQvkJzF8AtUA0SEbDccLvtWEIcMKpkQY4IFKruqGd1K0_qzZXVvoPrH
> k3ys7z
> >>> -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
> >>>
More information about the valhalla-dev
mailing list