RFR (M): 8034842: Parallelize the Free CSet phase in G1

[Thomas Schatzl]

Hi Derek and everybody,

On Thu, 2016-05-12 at 11:08 -0400, Derek White wrote:
> Hi Thomas,
> webrev.4/ looks good!
> 

  I had a lengthy discussion with Erik (and Mikael about this
particular topic) about this change yesterday: while he ultimately
approves of it (with some changes), I have been requested to split out
the part where the regions in the collection set are handled in an
array into a separate webrev.

It would be much better if the collection set for several reasons were
generally managed using an (explicit) array instead of the current
(HeapRegion-embedded) linked list.

So stay tuned for updated webrevs :)

Thanks for your review,
  Thomas

>  - Derek
> On 5/12/16 4:56 AM, Thomas Schatzl wrote:
> > Hi again,
> > 
> > On Wed, 2016-05-11 at 23:05 +0200, Thomas Schatzl wrote:
> > > Hi Derek,
> > > 
> > > On Wed, 2016-05-11 at 16:19 -0400, Derek White wrote:
> > > > Hi Thomas,
> > > > 
> > > > The changes in webrev.3 look good.
> > > > 
> > > > I have some comments on bigger picture now. I have one possible
> > > > bug,
> > > > and several increasingly annoying "you did it this way, why
> > > > didn't
> > > > you do it this way instead" comments :-)
> > > > 
> > > > src/share/vm/gc/g1/g1CollectedHeap.cpp:
> > > > 
> > > > - Is _regions_freed ever set? Can use local_free_list.length()
> > > > instead.
> > > I will look into this.
> > That has actually been a bug. Thanks for catching this.
> > 
> > > > - Could add evacuation_info parameter to complete_work(), and
> > > > remove the _evacuation_info field from G1FreeCollectionSetTask.
> > > Fine with me.
> > Done.
> > 
> > > > - do_serial_work() is done in parallel with do_parallel_work().
> > > > Is
> > > > that safe?  hr->rem_set()->clear_locked() or 
> > > > _g1h->_hot_card_cache->reset_card_counts(hr) can be called
> > > > before
> > > > or 
> > > > after _g1h->free_region(hr, &_local_free_list, true, true,
> > > > true).
> > > It is. The rem sets and the card counts are pretty independent
> > > data
> > > structures and can be removed any time.
> > > 
> > > > Line 4928:
> > > > - Is using the  _serial_work_claim mechanism safer than just
> > > > testing
> > > > for (worker_id == 0)? Because it's not simpler :-)
> > > Because worker 0 might not be actually the first one reaching the
> > > do_work() method, potentially unnecessarily delaying the entire
> > > process until it finished spinning up. While we always need to
> > > wait
> > > for all threads to spin up to end the task, using worker_id == 0
> > > means that we will at least need to wait for spin up of thread
> > > plus
> > > the serial work.
> > > > - I was going to question sorting the regions into young and
> > > > old
> > > > lists, and managing two lists instead of having just one. But
> > > > that
> > > > does simplify the G1GCPhaseTimes recording.
> > > This is the main reason.
> > > 
> > > I will provide a new webrev tomorrow.
> > http://cr.openjdk.java.net/~tschatzl/8034842/webrev.4/ (full)
> > http://cr.openjdk.java.net/~tschatzl/8034842/webrev.3_to_4/ (diff)
> > 
> > There are a few other changes found when walking through the code
> > with
> > Erik Helin, and then again by myselves.
> > 
> > Thanks,
> >   Thomas
> >