RFR 8223774: Shenandoah: Refactor ShenandoahRootProcessor and family

[Aleksey Shipilev]

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.

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

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

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

*) shenandoahArguments.cpp: this block is better be in ShenandoahTraversalHeuristics?

 186   if (strcmp(ShenandoahGCHeuristics, "traversal") == 0) {
 187     FLAG_SET_DEFAULT(ShenandoahConcurrentScanCodeRoots, false);
 188   }
 189

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


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


-Aleksey