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

[Aleksey Shipilev]

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);

*) 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.

*) 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?

*) In shenandoahRootProcessor.cpp, CLD roots are code roots now? Does it make sense?


-- 
Thanks,
-Aleksey