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

[Liu Xin]

hi, Felix,

The original storestore appears right before strb.  your membar(storeStore)
is after strb.  is it the same?
if it's same, we can place membar out of if/else to dedup.


diff --git a/src/hotspot/src/share/vm/c1/c1_LIRGenerator.cpp
b/src/hotspot/src/share/vm/c1/c1_LIRGenerator.cpp
index 19ec64d8..068f0797 100644
--- a/src/hotspot/src/share/vm/c1/c1_LIRGenerator.cpp
+++ b/src/hotspot/src/share/vm/c1/c1_LIRGenerator.cpp
@@ -1615,6 +1615,9 @@ void
LIRGenerator::CardTableModRef_post_barrier(LIR_OprDesc* addr, LIR_OprDesc*
               new LIR_Address(tmp, load_constant(card_table_base),
                               T_BYTE));
   }
+  if (UseConcMarkSweepGC && CMSPrecleaningEnabled) {
+      __ membar_storestore();
+  }
 #endif
 }

On Mon, Dec 16, 2019 at 3:21 AM Yangfei (Felix) <felix.yang at huawei.com>
wrote:

> 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
>