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

[Thomas Schatzl]

Hi everyone,

  there is a new version of this CR available at

http://cr.openjdk.java.net/~tschatzl/8034842/webrev.5/ (full)

Due to the changes afforded by JDK-8159978 there were a lot of changes
that made an incremental webrev useless. However I think this split-out 
made the change nicer (imo).

Testing:
jprt, vm.gc testlist

Thanks,
  Thomas

On Fri, 2016-05-13 at 13:04 +0200, Thomas Schatzl wrote:
> 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
> > >