RFR: Bitmap based ShHeapRegionSet

[Roman Kennke]

Hi Aleksey,

Thanks for reviewing!


> On 04/15/2018 09:18 PM, Roman Kennke wrote:
>> - We used a little nasty trick to implement degenerate update-refs: we
>> carry over the implicit state of iteration of ShHeapRegionSet
>> (current_index) from the concurrent phase to the STW degen phase to
>> finish off the remaining regions. This is changed a little bit by using
>> a ShenandoahHeap field _update_refs_iterator to keep the iterator intact
>> from init-update-refs to final-update-refs.
> 
> It does not look like a nasty trick: we are handing over the iteration state, and we do that cleanly
> by sharing the iterator itself. What we had before -- piggybacking on internal SHRS state -- is the
> nasty trick.

That is what I meant. I did a similar nasty trick in my initial
partia-traversal patch: I needed to scan roots during concurrent phase,
but not during final-traversal phase, but the code path was the same
(main_loop() ). So I reset the iterator in init-traversal,
conc-traversal would scan roots using that, and then in final-traversal
the iteration state would already be exhausted and roots not scanned
again. It's ugly because it relies on some implicit state somewhere.

>> - For ShenandoahRegionIterator I don't see the point to distinguish
>> between concurrent and single-threaded versions for next(): it's simply
>> using Atomic::add() and thus can be used for both, and the penalty for
>> single-threaded should be small and not worth the hassle because
>> single-threaded use doesn't seem *that* performance critical anyway.
> 
> I think there are places during pauses that we want to have single-threaded iterator, to
> micro-optimize the region traversal. Although I cannot spot them right away, I think we are doing
> index-based accesses in all important places. Something to keep for the future.

Yup.

>> - The new ShHeapRegionSet is basically like ShCollectionSet, but with
>> the cset specific stuff ripped out.
>>
>> Tests: hotspot_gc_shenandoah (release/fastdebug), specjvm
>> (release/fastdebug)
>>
>> http://cr.openjdk.java.net/~rkennke/bitmaphrs/webrev.00/
> 
> I like the way it turned out. Don't you like the absence of clear_claimed_index()?

Oh yes. That was a design mistake that stuck with us for too long.

> Minor nits:
> 
> *) ShenandoahHeap::iterate_regions() is misnomer, as it returns Iterator. Should be
> ShenandoahHeap::region_iterator().

Agreed. Fixed.

> *) Ditto: SHRS::iterate() -> SHRS::iterator()

Fixed too.

> *) ShenandoahHeapRegionSetIterator should extend StackObj to prevent accidental memory leak?

Good spot. Fixed.

> *) shenandoahHeap.hpp, "public" done twice:
> 
>  610 public:
>  611  public:

Fixed.

> *) shenandoahHeapRegionSet.hpp, comments are obsolete:
> 
>   64   // Add region to collection set
> 
>   68   // Remove region from collection set
> 

Ok. Changed to just 'set' rather than 'collection set'.

> *) shenandoahMarkCompact.cpp, indenting and double-;
> 
>  368       } else {
>  369       // Out of empty region? Compact within the same region.
>  370         new_to_region = _from_region;;
>  371       }

Fixed.

> *) shenandoahHeapRegionSet.inline.hpp, inline guard comment is out of sync:
> 
>   51 #endif // SHARE_VM_GC_SHENANDOAH_SHENANDOAHFASTREGIONSET_INLINE_HPP

Whoopsie. Fixed.

Diff:
http://cr.openjdk.java.net/~rkennke/bitmaphrs/webrev.01.diff/
Full:
http://cr.openjdk.java.net/~rkennke/bitmaphrs/webrev.01/

Better?

Thanks, Roman