RFR: Fold Partial GC into Traversal GC

[Roman Kennke]

A few more additions/fixes:
- move print_heap_regions() in ShenandoahHeap::init() down to the end,
when all stuff is initialized (esp. traversal_gc and its root/traversal
sets, if present).
- fix compilation of shenandoahAsserts
- call monitoring support partial or concurrent depending on current gc
cycle mode

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

Roman



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