[15] RFR 8239926: Shenandoah: Shenandoah needs to mark nmethod's metadata

[Zhengyu Gu]

There are two issues in earlier patch.

1) It keeps unloading nemthod's metadata alive
2) There are chances that GC is cancelled when it arrives final mark 
pause, that results bypassing evacuation/concurrent roots and triggers 
assertion in ShenandoahNMethod::heal_nmethod() method.

Now, I moved all nmethod metadata marking and evacuating inside 
ShenandoahNMethod::heal_nmethod(), which is cleaner.

Also, incorporated Aleksey's suggestion for handling nmethod disarming 
during degenerated GC, to follow regular phase transition.

Updated: http://cr.openjdk.java.net/~zgu/JDK-8239926/webrev.03/

Test:
   tier1 (fastdebug and release) with ShenandoahGC

Thanks,

-Zhengyu

On 3/13/20 8:56 AM, Zhengyu Gu wrote:
> Overnight tests showed problems with this patch. So, I would like to 
> withdraw this RFR.
> 
> Thanks,
> 
> -Zhengyu
> 
> On 3/12/20 12:43 PM, Roman Kennke wrote:
>>>> Hi Zhengyu,
>>>>
>>>> in src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp:
>>>> +      } else if
>>>> (ShenandoahConcurrentRoots::can_do_concurrent_class_unloading()) {
>>>> +        // Disarm nmethods that armed for concurrent mark.
>>>> +        // On normal code path (non-empty Cset), it depends on
>>>> update_roots() to
>>>> +        // disarm nmethods in degenerated GC.
>>>> +        ShenandoahCodeRoots::disarm_nmethods();
>>>>
>>>> beware that the update_roots() is only called at the end of update_refs
>>>> phase. The same call at end of marking is orphaned since removal of
>>>> piggy-backed marking.
>>>
>>> I think it is fine, successful degenerated GC cycle should always
>>> execute update_refs, no?
>>
>> Ok. I was only worried because the comment seems to imply it relies to
>> update_roots() at the end of mark. Aleksey's patch is removing that. If
>> update_roots() at the end of update_refs is good too, then fine.
>>
>> Thanks,
>> Roman
>>
>>> Thanks,
>>>
>>> -Zhengyu
>>>
>>>>
>>>> Otherwise looks good.
>>>>
>>>> Thanks,
>>>> Roman
>>>>
>>>>
>>>> On 3/11/20 10:43 PM, Zhengyu Gu wrote:
>>>>> Revised based on offline discussions.
>>>>>
>>>>> Piggyback on stack code root rescanning to SATB draining task.
>>>>>
>>>>> Webrev: http://cr.openjdk.java.net/~zgu/JDK-8239926/webrev.02/
>>>>>
>>>>> Reran tests:
>>>>>     hotspot_gc_shenandoah
>>>>>     tools/javac
>>>>>
>>>>> Thanks,
>>>>>
>>>>> -Zhengyu
>>>>>
>>>>> On 3/4/20 6:06 PM, Zhengyu Gu wrote:
>>>>>> Traversal GC has the same issue, also need to remark on stack code
>>>>>> roots in final traversal.
>>>>>>
>>>>>> @@ -263,11 +263,12 @@
>>>>>>         if (!_heap->is_degenerated_gc_in_progress()) {
>>>>>>           ShenandoahTraversalRootsClosure roots_cl(q, rp);
>>>>>>           ShenandoahTraversalSATBThreadsClosure tc(&satb_cl);
>>>>>>           if (unload_classes) {
>>>>>>             ShenandoahRemarkCLDClosure remark_cld_cl(&roots_cl);
>>>>>> -        _rp->strong_roots_do(worker_id, &roots_cl, &remark_cld_cl,
>>>>>> NULL, &tc);
>>>>>> +        MarkingCodeBlobClosure code_cl(&roots_cl,
>>>>>> CodeBlobToOopClosure::FixRelocations);
>>>>>> +        _rp->strong_roots_do(worker_id, &roots_cl, &remark_cld_cl,
>>>>>> &code_cl, &tc);
>>>>>>           } else {
>>>>>>             CLDToOopClosure cld_cl(&roots_cl,
>>>>>> ClassLoaderData::_claim_strong);
>>>>>>             _rp->roots_do(worker_id, &roots_cl, &cld_cl, NULL, &tc);
>>>>>>           }
>>>>>>         } else {
>>>>>>
>>>>>>
>>>>>> Updated webrev: 
>>>>>> http://cr.openjdk.java.net/~zgu/JDK-8239926/webrev.01/
>>>>>>
>>>>>> Thank,
>>>>>>
>>>>>> -Zhengyu
>>>>>>
>>>>>> On 2/25/20 12:13 PM, Zhengyu Gu wrote:
>>>>>>> Shenandoah encounters a few test failures with tools/javac. Verifier
>>>>>>> catches unmarked oops in nmethod's metadata during root 
>>>>>>> evacuation in
>>>>>>> final mark phase.
>>>>>>>
>>>>>>> The problem is that, Shenandoah marks on stack nmethods in init mark
>>>>>>> pause, but it does not mark nmethod's metadata during concurrent 
>>>>>>> mark
>>>>>>> phase, when new nmethod is about to be executed.
>>>>>>>
>>>>>>> The solution:
>>>>>>> 1) Use nmethod_entry_barrier to keep nmethod's metadata alive when
>>>>>>> the nmethod is about to be executed, when nmethod entry barrier is
>>>>>>> supported.
>>>>>>>
>>>>>>> 2) Remark on stack nmethod's metadata at final mark pause.
>>>>>>>
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8239926
>>>>>>> Webrev: http://cr.openjdk.java.net/~zgu/JDK-8239926/webrev.00/
>>>>>>>
>>>>>>> Test:
>>>>>>>      hotspot_gc_shenandoah (fastdebug and release)
>>>>>>>      tools/javac with ShenandoahCodeRootsStyle = 1 and 2 
>>>>>>> (fastdebug and
>>>>>>> release)
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> -Zhengyu
>>>>>
>>>>
>>>
>>