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

[Zhengyu Gu]

Hi Aleksey,

Please see comments inline.

On 3/16/20 1:00 PM, Aleksey Shipilev wrote:
> On 3/16/20 5:26 PM, Zhengyu Gu wrote:
>> Updated: http://cr.openjdk.java.net/~zgu/JDK-8239926/webrev.03/
> 
> Very good. Mostly stylistic, and tidy-ups comments:
> 
> *) Can we split the has_forwarded_object path in shenandoahConcurrentMark.cpp here? It would avoid
> instantiating both closures at expense of some code duplication:
> 
>   247         ShenandoahMarkResolveRefsClosure resolve_mark_cl(q, rp);
>   248         ShenandoahMarkRefsClosure mark_cl(q, rp);
>   249         OopClosure* oops = ShenandoahHeap::heap()->has_forwarded_objects() ?
>   250                            static_cast<OopClosure*>(&resolve_mark_cl) :
>   251                            static_cast<OopClosure*>(&mark_cl);
>   252         MarkingCodeBlobClosure blobsCl(oops, !CodeBlobToOopClosure::FixRelocations);
>   253         ShenandoahSATBAndRemarkCodeRootsThreadsClosure tc(&cl, &blobsCl);
>   254         Threads::threads_do(&tc);
> 

Sure.

> *) Please reformat the comment here in shenandoahHeap.cpp:
> 
> 1424   // Arm nmethods for concurrent marking.
> 1425   // When a nmethod is about to be executed, we need to make sure that all its
> 1426   // metadata are marked.
> 1427   // The alternative is to remark thread roots at final mark pause, but it can
> 1428   // be potential latency killer.
> 
> to:
> 
>     // Arm nmethods for concurrent marking. When a nmethod is about to be executed,
>     // we need to make sure that all its metadata are marked. alternative is to remark
>     // thread roots at final mark pause, but it can be potential latency killer.
> 

Fixed.

> *) In shenandoahNMethod.cpp, I do wonder if you want to specialize
> ShenandoahKeepNMethodMetadataAliveClosure with template HAS_FWD. And then dispatch to proper closure
> in ShenandoahNMethod::heal_nmethod. Keeps one branch out on the hot path for nmethod with many oops?
> 
Make sense.

> *) In shenandoahRootProcessor.cpp, CLD roots are code roots now? Does it make sense?
> 
Bad naming, changed:

_include_concurrent_roots => _stw_roots_processing
_include_concurrent_code_roots => _stw_class_unloading

Updated: http://cr.openjdk.java.net/~zgu/JDK-8239926/webrev.04/index.html

Test:
   In progress.

Okay?

Thanks,

-Zhengyu



>