[aarch64-port-dev ] RFR 8135018: AARCH64: Missing memory barriers for CMS collector
Liu Xin
navy.xliu at gmail.com
Mon Dec 16 22:22:50 UTC 2019
Hi, Reviewers,
How about this Webrev? I modified as Felix's suggestion.
https://cr.openjdk.java.net/~xliu/8135018/03/webrev/
Passed hotspot:tier1 and jcstress with CMS.
I eyeballed the generated code. it inserts membar(StoreStore) correctly.
0x0000ffff80322f50: lsr x8, x0, #3
0x0000ffff80322f54: str w8, [x1, #12]
0x0000ffff80322f58: lsr x0, x1, #9
0x0000ffff80322f5c: dmb ishst
0x0000ffff80322f60: mov x1, #0xf000 // #61440
0x0000ffff80322f64: movk x1, #0x8a49, lsl #16
0x0000ffff80322f68: movk x1, #0xffff, lsl #32
0x0000ffff80322f6c: strb wzr, [x0, x1, lsl #0] ;*putfield arr
; - ByteTest::actor1 at 4
(line 5)
thanks,
--lx
; - ByteTest::actor1 at 4 (lin
On Mon, Dec 16, 2019 at 3:54 AM Liu Xin <navy.xliu at gmail.com> wrote:
> "store store barriers"
> http://gee.cs.oswego.edu/dl/jmm/cookbook.html
>
> I think the JDK-8135018 inserts a membar(StoreStore) in the middle of
> put_field and update_cardTable on purpose. it can guarantee memory orders
> seen by Concurrent Mark Threads.
> if you put the storestore membar after two stores, IMHO, the hazard is
> still there.
>
>
> On Mon, Dec 16, 2019 at 3:34 AM Liu Xin <navy.xliu at gmail.com> wrote:
>
>> 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
>>>
>>
More information about the aarch64-port-dev
mailing list