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

[Derek White]

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.

- Could add evacuation_info parameter to complete_work(), and remove the 
_evacuation_info field from G1FreeCollectionSetTask.

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

Line 4928:
- Is using the  _serial_work_claim mechanism safer than just testing for 
(worker_id == 0)? Because it's not simpler :-)

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

  - Derek


  On 5/11/16 6:04 AM, Thomas Schatzl wrote:
> Hi Derek,
>
>    I posted new webrevs at
>
> http://cr.openjdk.java.net/~tschatzl/8034842/webrev.3/ (full)
> http://cr.openjdk.java.net/~tschatzl/8034842/webrev.2_to_3 (diff)
>
> incorporating most of your suggestions. See below for one comment:
>
> On Tue, 2016-05-10 at 19:00 -0400, Derek White wrote:
>> Hi Thomas,
>>
>> This is an initial pass of mostly minor comments. I'm still getting a
>> handle on the bigger picture.
>>
>> ----------------------------------
>> src/share/vm/gc/g1/heapRegionSet.hpp
>>    - Copyrights.
>>
>> ----------------------------------
>> src/share/vm/gc/g1/g1CollectedHeap.cpp:
>>
>> Lines 4733, 4735:
>> - Add comment about being array?
>>
>> Line 4744:
>> - Is "_pre_used" the same as "bytes (or words?) successfully
>> evacuated"?
>>
> I renamed the member variable.
>
>> Line 4752:
>>    - Consider making do_serial_work directly update G1GCPhaseTimes,
>> like
>> do_parallel_work() does. Then cleanup up
>> G1FreeCollectionSetTask::work(). Otherwise fix up spacing in
>> G1FreeCollectionSetTask::work()
>>
> Due to more recent experience with and where to put time reporting
> code, I now kind of dislike doing the timing within the method doing
> the actual work since it disallows you to reuse the methods. That bit
> me once too often.
>
> I also changed the do_parallel_work() etc. methods to not return the
> time taken any more.
>
> Thanks,
>    Thomas
>