RFR: 8216557 Aarch64: Add support for Concurrent Class Unloading
Per Liden
per.liden at oracle.com
Fri Mar 27 11:59:42 UTC 2020
Hi again,
On 3/27/20 12:36 PM, Per Liden wrote:
> Hi Stuart,
>
> Awesome, thanks a lot for doing this!
>
> On 3/26/20 11:42 PM, Stuart Monteith wrote:
>> Hello,
>> Please review this change to implement nmethod entry barriers on
>> aarch64, and hence concurrent class unloading with ZGC. Shenandoah will
>> need to be separately tested and enabled - there are problems with this
>> on Shenandoah.
>>
>> It has been tested with JTreg, runs with SPECjbb, gcbench, and Lucene as
>> well as Netbeans.
>>
>> In terms of interesting features:
>> With nmethod entry barriers, immediate oops are removed by:
>> LIR_Assembler::jobject2reg and MacroAssembler::movoop
>> This is to ensure consistency with the entry barrier, as
>> otherwise with
>> an immediate we'd otherwise need an ISB.
>>
>> I've added "-XX:DeoptNMethodBarrierALot". I found this
>> functionality
>> useful in testing as deoptimisation is very infrequent. I've written it
>> as an atomic to avoid it happening too frequently. As it is a new
>> option, I'm not sure whether any more is needed than this review. A new
>> test has been added
>> "test/hotspot/jtreg/gc/stress/gcbasher/TestGCBasherDeoptWithZ.java" to
>> test GC with that option enabled.
>>
>> BarrierSetAssembler::nmethod_entry_barrier
>> This method emits the barrier code. In internal review it was
>> suggested
>> the "dmb( ISHLD )" should be replaced by "membar(LoadLoad)". I've not
>> done this as the BarrierSetNMethod code checks the exact instruction
>> sequence, and I prefer to be explicit.
>>
>> Benchmarking method entry shows an increase of around 6ns
>> with the
>> nmethod entry barrier.
>>
>>
>> The deoptimisation code was contributed by Andrew Haley.
>>
>> The bug:
>> https://bugs.openjdk.java.net/browse/JDK-8216557
>>
>> The webrev:
>> http://cr.openjdk.java.net/~smonteith/8216557/webrev.0/
>
> I'll leave the aarch64-specific part for others to review. I just have
> two minor comments on the rest.
>
> * May I suggest that we rename DeoptNMethodBarrierALot to
> DeoptimizeNMethodBarriersALot, to better match -XX:DeoptimizeALot and
> friends?
>
> * The "counter" used should probably be an unsigned type, to avoid any
> overflow UB. That variable could also move into the scope where it's used.
>
> Like:
>
> ----------------------------------------------------------
> diff --git a/src/hotspot/share/gc/shared/barrierSetNMethod.cpp
> b/src/hotspot/share/gc/shared/barrierSetNMethod.cpp
> --- a/src/hotspot/share/gc/shared/barrierSetNMethod.cpp
> +++ b/src/hotspot/share/gc/shared/barrierSetNMethod.cpp
> @@ -50,7 +50,6 @@
> int BarrierSetNMethod::nmethod_stub_entry_barrier(address*
> return_address_ptr) {
> address return_address = *return_address_ptr;
> CodeBlob* cb = CodeCache::find_blob(return_address);
> - static volatile int counter=0;
>
> assert(cb != NULL, "invariant");
>
> @@ -67,8 +66,9 @@
>
> // Diagnostic option to force deoptimization 1 in 3 times. It is
> otherwise
> // a very rare event.
> - if (DeoptNMethodBarrierALot) {
> - if (Atomic::add(&counter, 1) % 3 == 0) {
> + if (DeoptimizeNMethodBarriersALot) {
> + static volatile uint32_t counter = 0;
> + if (Atomic::add(&counter, 1u) % 3 == 0) {
> may_enter = false;
> }
> }
> diff --git a/src/hotspot/share/runtime/globals.hpp
> b/src/hotspot/share/runtime/globals.hpp
> --- a/src/hotspot/share/runtime/globals.hpp
> +++ b/src/hotspot/share/runtime/globals.hpp
> @@ -2489,7 +2489,7 @@
> product(bool, UseEmptySlotsInSupers, true, \
> "Allow allocating fields in empty slots of
> super-classes") \
>
> \
> - diagnostic(bool, DeoptNMethodBarrierALot, false, \
> + diagnostic(bool, DeoptimizeNMethodBarriersALot, false, \
> "Make nmethod barriers deoptimise a lot.") \
>
> // Interface macros
> ----------------------------------------------------------
>
>
> * Instead of adding a new file for the test, we could just add a new
> section in the existing test.
>
> * The test also needs to supply -XX:+UnlockDiagnosticVMOptions.
Meh, forgot -XX:+UnlockDiagnosticVMOptions in my patch. Updated:
----------------------------------------------------------
diff --git
a/test/hotspot/jtreg/gc/stress/gcbasher/TestGCBasherWithZ.java
b/test/hotspot/jtreg/gc/stress/gcbasher/TestGCBasherWithZ.java
--- a/test/hotspot/jtreg/gc/stress/gcbasher/TestGCBasherWithZ.java
+++ b/test/hotspot/jtreg/gc/stress/gcbasher/TestGCBasherWithZ.java
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2016, 2019, Oracle and/or its affiliates. All rights
reserved.
+ * Copyright (c) 2016, 2020, Oracle and/or its affiliates. All rights
reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -35,6 +35,18 @@
* @summary Stress ZGC
* @run main/othervm/timeout=200 -Xlog:gc*=info -Xmx384m -server
-XX:+UnlockExperimentalVMOptions -XX:+UseZGC
gc.stress.gcbasher.TestGCBasherWithZ 120000
*/
+
+/*
+ * @test TestGCBasherDeoptWithZ
+ * @key gc stress
+ * @library /
+ * @requires vm.gc.Z
+ * @requires vm.flavor == "server" & !vm.emulatedClient &
!vm.graal.enabled & vm.opt.ClassUnloading != false
+ * @summary Stress ZGC with nmethod barrier forced deoptimization enabled
+ * @run main/othervm/timeout=200 -Xlog:gc*,nmethod+barrier=trace
-Xmx384m -XX:+UnlockExperimentalVMOptions -XX:+UseZGC
+ * -XX:+UnlockDiagnosticVMOptions
-XX:+DeoptimizeNMethodBarriersALot -XX:-Inline
gc.stress.gcbasher.TestGCBasherWithZ 120000
+ */
+
public class TestGCBasherWithZ {
public static void main(String[] args) throws IOException {
TestGCBasher.main(args);
----------------------------------------------------------
cheers,
Per
>
> Like:
>
> ----------------------------------------------------------
> diff --git
> a/test/hotspot/jtreg/gc/stress/gcbasher/TestGCBasherWithZ.java
> b/test/hotspot/jtreg/gc/stress/gcbasher/TestGCBasherWithZ.java
> --- a/test/hotspot/jtreg/gc/stress/gcbasher/TestGCBasherWithZ.java
> +++ b/test/hotspot/jtreg/gc/stress/gcbasher/TestGCBasherWithZ.java
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2016, 2019, Oracle and/or its affiliates. All rights
> reserved.
> + * Copyright (c) 2016, 2020, Oracle and/or its affiliates. All rights
> reserved.
> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> *
> * This code is free software; you can redistribute it and/or modify it
> @@ -35,6 +35,18 @@
> * @summary Stress ZGC
> * @run main/othervm/timeout=200 -Xlog:gc*=info -Xmx384m -server
> -XX:+UnlockExperimentalVMOptions -XX:+UseZGC
> gc.stress.gcbasher.TestGCBasherWithZ 120000
> */
> +
> +/*
> + * @test TestGCBasherDeoptWithZ
> + * @key gc stress
> + * @library /
> + * @requires vm.gc.Z
> + * @requires vm.flavor == "server" & !vm.emulatedClient &
> !vm.graal.enabled & vm.opt.ClassUnloading != false
> + * @summary Stress ZGC with nmethod barrier forced deoptimization enabled
> + * @run main/othervm/timeout=200 -Xlog:gc*,nmethod+barrier=trace
> -Xmx384m -XX:+UnlockExperimentalVMOptions -XX:+UseZGC
> + * -XX:+DeoptimizeNMethodBarriersALot
> -XX:-Inline gc.stress.gcbasher.TestGCBasherWithZ 120000
> + */
> +
> public class TestGCBasherWithZ {
> public static void main(String[] args) throws IOException {
> TestGCBasher.main(args);
> ----------------------------------------------------------
>
> cheers,
> Per
>
>
>>
>>
>> BR,
>> Stuart
>> IMPORTANT NOTICE: The contents of this email and any attachments are
>> confidential and may also be privileged. If you are not the intended
>> recipient, please notify the sender immediately and do not disclose
>> the contents to any other person, use it for any purpose, or store or
>> copy the information in any medium. Thank you.
>>
More information about the zgc-dev
mailing list