RFR (M): 8159978: Use an array to store the collection set regions instead of linking through regions

[Thomas Schatzl]

Hi,

On Tue, 2016-06-28 at 15:41 -0700, Jon Masamitsu wrote:
> Thomas,
> 
> http://cr.openjdk.java.net/~tschatzl/8159978/webrev.1/src/share/vm/gc
> /g1/g1CollectedHeap.cpp.frames.html
> 
> For an evacuation failure the region is set to old here
> 
> 4664       // The region is now considered to be old.
> 4665       r->set_old();
> 
> and below the elapsed times are incremented.
> 
> 4676     if (r->is_young()) {
> 4677       _young_time += os::elapsedTime() - start_time;
> 4678     } else {
> 4679       _non_young_time += os::elapsedTime() - start_time;
> 4680     }
> 
> Is it correct that elapsed time for a failed evacuation will be now
> be added to the _non_young_time?  Whereas before it was included in
> the _young_time because non_young was assigned a value before the
> evacuation failure was tested?

Unintended change. Fixed.

It would have been correct again in the follow-up change that
parallelizes collection set freeing, so that's why I probably
overlooked this. Sorry.


> [..]

> http://cr.openjdk.java.net/~tschatzl/8159978/webrev.1/src/share/vm/gc
> /g1/g1CollectionSet.cpp.frames.html
> 
>   177 void G1CollectionSet::iterate_from(HeapRegionClosure* cl, uint
> worker_id, uint total_workers, bool may_be_aborted) {
> 
> I don't understand this iterate method.  It seems like because of
> line 196
> 
>  195     if (cur_pos == len) {
>  196       cur_pos = 0;
>  197     }
> 
> all the workers will process region 0 in the collection set.  Is this
> method creating slices for each worker?  Or trying to stride through
> the collection set with each worker getting a different starting
> index?

The intent is to let different threads iterate over the entire list
with each starting at a different index.

I updated the change at
http://cr.openjdk.java.net/~tschatzl/8159978/webrev.2/ (full)
http://cr.openjdk.java.net/~tschatzl/8159978/webrev.1_to_2/ (diff)

Changes include (from Erik Helin):

- rename G1CollectionSet::set_max_size() to
G1CollectionSet::initialize() because generally the post-construction
method in our code base typically is called that.

- removed the may_be_aborted parameter
from G1CollectionSet::iterate_from(), as it was basically used only for
paranoia checking code.

- closurified the G1CollectionSet::verify_young_ages()
and G1CollectionSet::verify_young_cset_indices() methods.

- some comments about MT safeness in g1CollectionSet.hpp

- constified the G1CollectionSet::iterate* methods

- fixed timing attribution

Testing:
jprt

Thanks,
  Thomas