RFR(L) 8228665: Shenandoah: Implementation of Concurrent nmethod evacuation

[Roman Kennke]

Hi Zhengyu,

It still looks good. Thanks!

Roman

> Hi Roman,
> 
> A couple of updates:
> 
> 1) Refactored nmethod iteration according to the changes made to
> JDK-8227103. Now, nmethod table iterations are handled by
> ShenandoahNMethodTableSnapshot.
> 
> 2) hotspot_gc_shenandoah tests passed on MacOS X, therefore, concurrent
> nmethod evacuation is enabled for MacOS X now.
> 
> Updated webrev: http://cr.openjdk.java.net/~zgu/JDK-8228665/webrev.02/
> 
> Test:
>   hotspot_gc_shenandoah (fastdebug and release) on Linux x86_64
>   specjvm on Linux x86_64
>   hotspot_gc_shenandoah (fastdebug and release) on MacOS X
> 
> Thanks,
> 
> -Zhengyu
> 
> 
> On 7/30/19 12:43 PM, Roman Kennke wrote:
>> Ok, looks good to me now. Thanks!
>>
>> Roman
>>
>>> On 7/29/19 12:25 PM, Roman Kennke wrote:
>>>> Hi Zhengyu,
>>>>
>>>> First glance (will need to do another review pass later):
>>>>
>>>> - Does it not also need an aarch64 port?
>>>> - Does it also work on x86_32?
>>>
>>> Yes, but I guess we have to deal with them later. Let's get Linux x86_64
>>> and Windows 64 in and stabilize them first.
>>>
>>>
>>>> - src/hotspot/share/gc/shenandoah/shenandoahArguments.cpp:
>>>>
>>>> Why is that change there?
>>>>
>>>> -  if (!ClassUnloading ||
>>>> !FLAG_IS_CMDLINE(ClassUnloadingWithConcurrentMark)) {
>>>> +  if (!ClassUnloading) {
>>>>        log_info(gc)("Consider -XX:+ClassUnloadingWithConcurrentMark if
>>>> large pause times "
>>>>                     "are observed on class-unloading sensitive
>>>> workloads");
>>>
>>> We do want to honor ClassUnloadingWithConcurrentMark's default value, if
>>> we can do concurrent class unloading.
>>>
>>> However, I misplaced this change here, should belong to concurrent class
>>> unloading changeset.
>>>
>>>>
>>>> - src/hotspot/share/gc/shenandoah/shenandoahClosures.hpp:
>>>>
>>>> +  BarrierSetNMethod* _bs;
>>>>
>>>> Make this BarrierSetNMethod* const _bs. Not sure if it makes a
>>>> difference perf-wise, but it doesn't hurt and might give the compiler a
>>>> hint.
>>>>
>>> Okay.
>>>
>>>> - I guess this partly answers the questions above:
>>>>
>>>> +bool ShenandoahConcurrentRoots::can_do_concurrent_nmethods() {
>>>> +#if defined(_LP64) && defined(X86) && (defined(LINUX) ||
>>>> defined(_WINDOWS))
>>>>
>>>> But why do we deal with LINUX and WINDOWS there? Is there anything OS
>>>> specific in this codes?
>>>
>>> I guess MacOS X and Solaris X86?
>>>
>>>>
>>>> - This change will enable class-unloading on every cycle, is that
>>>> right?
>>>>
>>>
>>> Right. Again, It should belong to next changeset.
>>>
>>> Updated: http://cr.openjdk.java.net/~zgu/JDK-8228665/webrev.01/
>>>
>>> Reran hotspot_gc_shenandoah tests
>>>
>>> Thanks,
>>>
>>> -Zhengyu
>>>
>>>
>>>
>>>>
>>>> Thanks a lot for doing this! This is huge!
>>>> Roman
>>>>
>>>>
>>>>> This patch moves nmethod evacuation and disarming into concurrent
>>>>> phase.
>>>>> However, nmethod's liveness is still checked at a pause.
>>>>>
>>>>> This is the last step before we complete concurrent class unloading,
>>>>> where we move class loader and nmethod's liveness check into
>>>>> concurrent
>>>>> evacuation phase.
>>>>>
>>>>> Given the scope of this changes, let's bake it in shenandoah/jdk
>>>>> first,
>>>>> before upstream to jdk/jdk.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8228665
>>>>> Webrev: http://cr.openjdk.java.net/~zgu/JDK-8228665/webrev.00/
>>>>>
>>>>> Test:
>>>>>     hotspot_gc_shenandoah (fastdebug and release)
>>>>>     specjvm (fastdebug and release)
>>>>>
>>>>> Thanks,
>>>>>
>>>>> -Zhengyu
>>>>
>>