RFR: 8216557 Aarch64: Add support for Concurrent Class Unloading

[Erik Österlund]

Hi Stuart,

Thanks for sorting this out on AArch64. It is nice to see thatyou can 
implement these
barriers on platforms that do not have instruction cache coherency.

One small change request:
It looks like in C1 you inject the entry barrier right after build_frame 
is done:

  629       build_frame();
  630       {
  631         // Insert nmethod entry barrier into frame.
  632         BarrierSetAssembler* bs = 
BarrierSet::barrier_set()->barrier_set_assembler();
  633         bs->nmethod_entry_barrier(_masm);
  634       }

Unfortunately, this is in the platform independent part of the LIR 
assembler. In the x86 version
we inject it at the very end of build_frame() instead, which is a 
platform-specific function.
The platform-specific function is in the C1 macro assembler file for 
that platform.

We intentionally put it in the platform-specific path as it is a 
platform-specific feature.
Now on x86, the barrier code will be emitted once in build_frame() and 
once after returning
from build_frame, resulting in two nmethod entry barriers, and only the 
first one will get
patched, causing the second one to mostly take slow paths, which isn't 
necessarily wrong,
but will cause regressions.

I would propose you just move those lines into the very end of the 
AArch64-specific part of
build_frame().

I don't need to see another webrev for that trivial code motion. This 
looks good to me.
Agan, thanks a lot for fixing this! It will allow me to go forward with 
concurrent stack
scanning on AArch64 as well.

Thanks,
/Erik


On 2020-03-26 23:42, 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/
>
>
> 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.