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

[Erik Helin]

On 2016-06-29, Thomas Schatzl wrote:
> 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)

Looks good, Reviewed. Nice work Thomas!

Erik

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