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

[Thomas Schatzl]

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