RFR: 8216557 Aarch64: Add support for Concurrent Class Unloading

[Stuart Monteith]

Thanks Per,
        That all makes sense - I've made those changes, they'll appear in the next patch set.


On 27/03/2020 11:36, 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.
>
> 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.
>>

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.