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

[Yangfei (Felix)]

Yes, I noticed that  ☺

https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2019-December/008389.html

From: Liu Xin [mailto:navy.xliu at gmail.com]
Sent: Monday, December 16, 2019 7:35 PM
To: Yangfei (Felix) <felix.yang at huawei.com>
Cc: aarch64-port-dev at openjdk.java.net; Andrew Haley <aph at redhat.com>; Doerr, Martin <martin.doerr at sap.com>
Subject: Re: [aarch64-port-dev ] RFR 8135018: AARCH64: Missing memory barriers for CMS collector

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<mailto: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<mailto:navy.xliu at gmail.com>>
> Sent: Montag, 16. Dezember 2019 10:48
> To: Andrew Haley <aph at redhat.com<mailto:aph at redhat.com>>
> Cc: Doerr, Martin <martin.doerr at sap.com<mailto:martin.doerr at sap.com>>;
> aarch64-port-dev at openjdk.java.net<mailto:aarch64-port-dev at openjdk.java.net>; Hohensee, Paul
> <hohensee at amazon.com<mailto: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><mailto: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