RFR 8223774: Shenandoah: Refactor ShenandoahRootProcessor and family

[Zhengyu Gu]

Thanks for reviewing.

On 5/16/19 6:09 AM, Aleksey Shipilev wrote:
> On 5/13/19 5:00 PM, Zhengyu Gu wrote:
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8223774
>> Webrev: http://cr.openjdk.java.net/~zgu/JDK-8223774/webrev.00/index.html
> 
> Oh, this is a cute refactoring!
> 
> First sweep:
> 
> *) Maybe make ShenandoahRoot{Processor,Evacuator,Updater} the superclass and give them
> ShenandoahHeap in a const field? Then replace uses of ShenandoahHeap::heap() everywhere in them.
> Maybe even move phase_timings() and safepoint asserts there.
> 
Done.

> *) Is there really a significant difference between ShenandoahRootAdjuster and
> ShenandoahRootUpdater? Current thing is fine, just wanted to understand if they are common enough to
> merge.

Not at this point, but will be with concurrent stuffs. Let's revisit 
this after that.

> 
> *) I think names should be "Root*s*" for e.g. ShenandoahClassLoaderDataRoot, etc.
> 
Done.

> *) "const" on new fields, where applicable?
Done

> 
> *) shenandoahArguments.cpp: this block is better be in ShenandoahTraversalHeuristics?
> 
>   186   if (strcmp(ShenandoahGCHeuristics, "traversal") == 0) {
>   187     FLAG_SET_DEFAULT(ShenandoahConcurrentScanCodeRoots, false);
>   188   }
>   189

We don't have this heuristic, let's do it in follow up RFE.

> 
> *) This comment mentions ShenandoahConcurrentScanCodeRoots, but there are no uses in the subsequent
> block. Need to move point (c) to somewhere else?
> 
>   118  //   c. With ShenandoahConcurrentScanCodeRoots, we avoid scanning the entire code cache here,
>   119  //      and instead do that in concurrent phase under the relevant lock. This saves init mark
>   120  //      pause time.
>

Moved to right place.

> 
> *) Indents:
> 
>   154 ShenandoahRootProcessor::ShenandoahRootProcessor(uint n_workers, ShenandoahPhaseTimings::Phase
> phase) :
>   155   _thread_roots(n_workers > 1),
>   156    _phase(phase) {
>   157   assert(SafepointSynchronize::is_at_safepoint(), "Must at safepoint");
> 
> *) Stick with one message here? E.g. "Should be during class unloading".
> 
>   178   assert(!ShenandoahHeap::heap()->unload_classes(), "Use without class unloading");
> ...
>   192   assert(ShenandoahHeap::heap()->unload_classes(), "Use with class unloading cycle");
> 
Done.

Updated: http://cr.openjdk.java.net/~zgu/JDK-8223774/webrev.01/

Reran hotspot_gc_shenandoah (fastdebug and release)

Thanks,

-Zhengyu

> 
> -Aleksey
>