[aarch64-port-dev ] RFR 8135018: AARCH64: Missing memory barriers for CMS collector

[Yangfei (Felix)]

Sorry, I think I made a mistake for the patch in my previous mail.  

Should be:

diff -r 09d4b646f756 src/share/vm/c1/c1_LIRGenerator.cpp
--- a/src/share/vm/c1/c1_LIRGenerator.cpp       Tue Nov 12 17:54:52 2019 +0800
+++ b/src/share/vm/c1/c1_LIRGenerator.cpp       Mon Dec 16 19:24:38 2019 +0800
@@ -1619,6 +1619,9 @@
   } else {
     __ unsigned_shift_right(addr, CardTableModRefBS::card_shift, tmp);
   }
+  if (UseConcMarkSweepGC && CMSPrecleaningEnabled) {
+    __ membar_storestore();
+  }
   if (can_inline_as_constant(card_table_base)) {
     __ move(LIR_OprFact::intConst(0),
               new LIR_Address(tmp, card_table_base->as_jint(), T_BYTE));



> 
> Hi Liu Xin,
> 
>   Based on what Andrew said about UseCondCardMark, I agree with Martin
> that we go with your new backport webrev for 8135018 first.
>   My only concern is that we changed to store
> CardTableModRefBS::dirty_card_val() instead of constant zero to card_addr in
> c1_LIRGenerator.cpp.
>   It looks to me unnecessary to add this change when you are adding the
> missing memory bars.
>   So I suggest we do:
> 
>   diff -r 09d4b646f756 src/share/vm/c1/c1_LIRGenerator.cpp
> --- a/src/share/vm/c1/c1_LIRGenerator.cpp       Tue Nov 12 17:54:52 2019
> +0800
> +++ b/src/share/vm/c1/c1_LIRGenerator.cpp       Mon Dec 16 19:17:51
> 2019 +0800
> @@ -1622,10 +1622,16 @@
>    if (can_inline_as_constant(card_table_base)) {
>      __ move(LIR_OprFact::intConst(0),
>                new LIR_Address(tmp, card_table_base->as_jint(),
> T_BYTE));
> +    if (UseConcMarkSweepGC && CMSPrecleaningEnabled) {
> +      __ membar_storestore();
> +    }
>    } else {
>      __ move(LIR_OprFact::intConst(0),
>                new LIR_Address(tmp, load_constant(card_table_base),
>                                T_BYTE));
> +    if (UseConcMarkSweepGC && CMSPrecleaningEnabled) {
> +      __ membar_storestore();
> +    }
>    }
>  #endif
>  }
> 
> 
> Thanks,
> Felix
> 
> >
> > From: Liu Xin <navy.xliu at gmail.com>
> > Sent: Montag, 16. Dezember 2019 10:48
> > To: Andrew Haley <aph at redhat.com>
> > Cc: Doerr, Martin <martin.doerr at sap.com>;
> > aarch64-port-dev at openjdk.java.net; Hohensee, Paul
> > <hohensee at amazon.com>
> > Subject: Re: RFR 8135018: AARCH64: Missing memory barriers for CMS
> > collector
> >
> > Hi, Andrew and Felix,
> >
> > I did backport as Felix suggested. To get UseCondCardMark, we need to
> > backport other 3 patches.
> >
> > hg qseries as follows ( applied in order)
> > 8076987: https://cr.openjdk.java.net/~xliu/8076987/00/webrev/
> > 8078438: https://cr.openjdk.java.net/~xliu/8078438/00/webrev/
> > 8079315: https://cr.openjdk.java.net/~xliu/8079315/00/webrev/
> > 8135018: https://cr.openjdk.java.net/~xliu/8135018/02/webrev/
> >
> > As Andrew pointed out, I also notice that it introduces a storeLoad
> > membar for interpreter if UseCondCardMark is on.
> > The reason I didn't backport UseCondCardMark at first place because
> > it's off by default. I need to run all tests again with the new
> > parameter -XX:+UseCondCardMark to validate it.  Another reason is that
> > all performance evaluation was done on x86_64.
> > It's really time consuming to evaluate performance impact on aarch64.
> > So I shortcut to fix my primary bug.
> >
> > Could you sponsor my previous webrev?
> > https://cr.openjdk.java.net/~xliu/8135018/01/webrev/
> >
> > It does have a conflict with c1_LIRGenerator.cpp. How about I send
> > another webrev to jdk8u and update it as well?
> >
> > thanks,
> > --lx
> >
> >
> > On Mon, Dec 16, 2019 at 1:00 AM Andrew Haley
> > <aph at redhat.com<mailto:aph at redhat.com>> wrote:
> > On 12/14/19 4:47 AM, Liu Xin wrote:
> > > I trace the feature UseCondCardMark. It was introduced by the
> > > following changes.
> > > https://bugs.openjdk.java.net/browse/JDK-8076987 C1
> > > https://bugs.openjdk.java.net/browse/JDK-8078438 interpreter
> > > UseCondCardMark is only supported by C2 currently.
> >
> > UseCondCardMark was supposed to be a performance enhancement for very
> > large multi-core systems, but we realized that it needed a StoreLoad barrier.
> > Because StoreLoad is so expensive there is no performance advantage,
> > therefore as far as I can see the whole UseCondCardMark exercise is a
> > waste of time. There is no point back-porting changes to the interpreter and
> C1.
> >
> > Unless someone does some performance measurements to prove that there
> > is some performance gain in C1 and the interpreter, UseCondCardMark
> > changes should not be back-ported.
> >
> > --
> > Andrew Haley  (he/him)
> > Java Platform Lead Engineer
> > Red Hat UK Ltd. <https://www.redhat.com>
> > https://keybase.io/andrewhaley
> > EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671