RFR: Fold Partial GC into Traversal GC

[Aleksey Shipilev]

On 04/12/2018 10:58 AM, Roman Kennke wrote:
> Back then, the Traversal GC mode started out as special configuration of
> partial GC (i.e. put all used regions into cset and none into root set).
> 
> Then I forked partial GC to put in some extras without disturbing
> existing code, and because it needed slight changes to the algorithm
> that didn't really fit with partial GC.
> 
> Now I'm proposing to fold the partial mode into Traversal GC, and make
> Traversal GC the intermediate GC for partial. This has some great
> advantages:

This is cool! I think we need to do push a few preparatory things (some forked from this patch), see
below.

> Based on some earlier discussions with Aleksey on IRC I wanted to fold it into
> ShenandoahHeapRegionSet, but this felt awkward because ShHRS is not really a set, but a list,
> and it's sortable (and used for sorting), etc. So I left it as ShFastRegionSet for now to give
> some basis for discussions.

So, we need to squash the "sorting" thing. It seems SHRS::sort is used in partial ShCollectorPolicy.
Which means we can instead do the same thing we do non-partial ShCollectorPolicy: introduce another
struct like ShenandoahHeuristics::RegionData, sort the array of those structs, not the SHRS itself.
This unties the sorting knot, and allows folding ShFastRegionSet to SHRS.

Also, I think that SHRS changes should be in separate changesets, because they are backportable.
Even if backports are not using this new *Fast thing yet, there is no telling if they would in
future, which makes it more prudent to backport.

> http://cr.openjdk.java.net/~rkennke/partial-traversal/webrev.00/

Comments:

 *) markBitMap.hpp: missing comment for the new method

 *) SBS::ArrayCopyStoreValMode::WRITE_BARRIER_ALWAYS_ENQUEUE is just WRITE_BARRIER now?

 *) Even though it looks benign, the change in ShenandoahPassiveHeuristics should be separated for
backports:
  406     SHENANDOAH_ERGO_DISABLE_FLAG(ShenandoahStoreValEnqueueBarrier);

 *) The way the "minority" of collection is figured out is rather fragile. We should not write
anything in should_start_traversal_gc(), that method is supposed to be idempotent, and it can be
potentially called multiple times. As the rule, Heuristics should not carry GC state -- it has
fields for "caches", but not for the actual decisions. Maybe the split between _minor and _major
heuristics that we have right now is better.

 *) shenandoahFreeSet.cpp: there is _heap field, no need to use ShenandoahHeap::heap()

 *) Use line breaks! That would highlight that you have comments lurking inside the code blocks, see
e.g. ShenandoahResetNextBitmapTraversalTask::work


 *) shenandoahHeap.hpp: indenting:

  462   ShenandoahFastRegionSet* traversal_set() const { return _traversal_set; }

 *) ShenandoahHeapRegion::recycle(): there is the assert at L525 that is checking the bitmap is
clear. But we don't clear the bitmap when ShenandoahRecycleClearsBitmap is false? How that assert is
not failing? Is it cleared by some other path? I think we might be better off splitting this thing
into a separate change to review.

 *) Is the move of reset_next_mark_bitmap() in ShenandoahMarkCompact::phase4_compact_objects
meaningful? If so, we are better split it out, so backports are in sync.

 *) ShenandoahPacer::report_partial -- do we need it now?

 *) ShenandoahTraversalGC::process_oop, why the assert is commented?

 *) ShenandoahTraversalGC::process_oop, "oop previous" is dangling?

 *) I think we better write a few unit tests for BitMap::copy_from. There is already test_bitMap.cpp
which we can reuse.

Thanks,
-Aleksey