RFR: Fold Partial GC into Traversal GC

[Roman Kennke]

Re-based on latest merged repo:
http://cr.openjdk.java.net/~rkennke/partial-traversal/webrev.03/

Same content though.

Can I get a review?

Roman


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