RFR: Bitmap based ShHeapRegionSet

[Aleksey Shipilev]

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.

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

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

Minor nits:

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

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

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

*) shenandoahHeap.hpp, "public" done twice:

 610 public:
 611  public:

*) shenandoahHeapRegionSet.hpp, comments are obsolete:

  64   // Add region to collection set

  68   // Remove region from 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       }

*) shenandoahHeapRegionSet.inline.hpp, inline guard comment is out of sync:

  51 #endif // SHARE_VM_GC_SHENANDOAH_SHENANDOAHFASTREGIONSET_INLINE_HPP


Thanks,
-Aleksey