RFR: Fold Partial GC into Traversal GC

[Roman Kennke]

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

Yep. SHRS has been pushed separately.

>> http://cr.openjdk.java.net/~rkennke/partial-traversal/webrev.00/
> 
> Comments:
> 
>  *) markBitMap.hpp: missing comment for the new method

Ok, added.


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

Yep. Fixed.

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

You already pushed this too.

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

No, because the only way we can make a distinction now is that minor
used to be partial and major used to be adaptive. Now all is traversal.
But I agree, the way I did is seems fragile. I changed to so that
should_start_traversal() returns either NONE, MINOR or MAJOR, and that
is then set in ShHeap::set_cycle_mode(), and ShHeap::is_minor_gc() and
ShHeap::is_major_gc() tell what is currently going on. This will have to
be revisited once we allow interleaving minor and major GC though. Ok?

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

Ok, fixed.

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

Uh. Fixed.

>  *) shenandoahHeap.hpp: indenting:
> 
>   462   ShenandoahFastRegionSet* traversal_set() const { return _traversal_set; }
> 

I moved that stuff into ShenandoahTraversalGC because that is the only
place where it's used.

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

This is a tricky bugger. Normally (non-traversal), the complete bitmap
of trashed regions is clear because marking has not found anything in
there (that's why it's trashed). I tried to do the same with traversal
through normal mechanics: copy next -> complete even for trash regions,
or fill complete with 0, which should have the same effect, during
concurent-cleanup, and then it should be clear when recycling. However,
allocations can help out with recycling before conc-cleanup gets there.
That's why I moved this trash-complete-bitmap-cleaning into recycle. Do
you have a better idea?

>  *) Is the move of reset_next_mark_bitmap() in ShenandoahMarkCompact::phase4_compact_objects
> meaningful?

No. Reverted.


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

You tell me. I am absolutely not sure what to do about the pacer.

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

For no good reason. Re-instated.

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

WHoops. This was meant to be used for deciding whether or not we need to
update the matrix below. If another thread beat us updating the field,
we don't need to update the matrix. Fixed.

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

I've added a bunch of tests. And good that I did, it turned up a bug in
the impl :-) I fixed that, added comments and asserts too.

Some other stuff:
- previous patch allowed to cancel during root-scan. this is dangerous,
because degen (currently) does not pick up root scanning. I fixed that
by not allowing cancellation during root-scanning. We might want to
either add root scanning to degen-traversal, or probably even better,
interleave root-scanning with normal marking, which would have the
additional advantage to lower pressure on the task queues.
- I added in-traversal-set and in-root-set printout to shenandoahAsserts.cpp
- It turns out that we no longer need the out param 'bool evac' in
ShHeap::evacuate_object(). I removed that.


Differential:
http://cr.openjdk.java.net/~rkennke/partial-traversal/webrev.01.diff/
Full:
http://cr.openjdk.java.net/~rkennke/partial-traversal/webrev.01/

Better?