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

[Jon Masamitsu]

Thomas,

Looks correct as is.  Just some comments about readability
for your considerations.

http://cr.openjdk.java.net/~tschatzl/8034842/webrev.6/src/share/vm/gc/g1/g1CollectedHeap.cpp.frames.html

I found this a bit hard to read.

> 4821 size_t cur = Atomic::add(chunk_size(), &_parallel_work_claim) - 
> chunk_size();

I'd suggest

end = Atomic::add(chunk_size(), &_parallel_work_claim);
size_t cur = end - chunk_size();

and then at  4828

end = MIN2(end, _num_work_items);


Do you have any concerns here about having to load the
heap and then having to load the policy - that is enough
concerns to add a _g1h and _g1_policy and initialize them
in prepare_work()?

4804 G1GCPhaseTimes* timer = 
G1CollectedHeap::heap()->g1_policy()->phase_times();


Would 1U work here in place of (size_t) 1.

4860 uint const num_chunks = (uint)MAX2(_collection_set.region_length() 
/ G1FreeCollectionSetTask::chunk_size(), (size_t)1);


Could you explain your choice of "32" here.  Even just to say
experiments suggested that 32 was a good number or just
based on general experience, it seems like a good number.
It will save future generations from wondering.

4801 static size_t chunk_size() { return 32; } Would the comment "Claim 
parallel work" be better just before line 4821? Since that is where the 
chunks are claimed? Or should that comment be "Start parallel work"?
// Claim parallel work. 4821 size_t cur = Atomic::add(chunk_size(), 
&_parallel_work_claim) - chunk_size();


http://cr.openjdk.java.net/~tschatzl/8034842/webrev.6/src/share/vm/gc/g1/g1CollectedHeap.hpp.frames.html

Would "skip_remset" and "skip_hot_card_cache" be better names?
Just to avoid any implication that the RS and hot cache are needed
later.

653 bool keep_remset,
654 bool keep_hot_card_cache = false,


That's all.

Jon

On 07/08/2016 03:53 AM, Thomas Schatzl wrote:
> Hi all,
>
> On Tue, 2016-06-28 at 13:02 +0200, Thomas Schatzl wrote:
>
>              ^--- ping!
>
>> 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
> Erik H. had some minor comments (not a full review, just some
> unnecessary additions), and I also fixed some whitespace issues in this
> new revision:
>
> http://cr.openjdk.java.net/~tschatzl/8034842/webrev.6/ (full)
> http://cr.openjdk.java.net/~tschatzl/8034842/webrev.5_to_6/ (diff)
>
> Thanks,
>    Thomas
>