[aarch64-port-dev ] RFR: 8216557 Aarch64: Add support for Concurrent Class Unloading

[Erik Österlund]

Hi Stuart,

This looks good to me.

Thanks,
/Erik

On 2020-04-20 16:19, Stuart Monteith wrote:
> Hi,
> 	If anyone has bandwidth, would the be able to review this patch? It
> addresses Andrew, Per and Erik's comments:
>          http://cr.openjdk.java.net/~smonteith/8216557/webrev.1/
>
> Thanks,
> 	Stuart
>
>
> On 27/03/2020 09:47, Erik Österlund wrote:
>> 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