RFR (M): 8159978: Use an array to store the collection set regions instead of linking through regions

[Jon Masamitsu]

On 06/29/2016 05:32 AM, Thomas Schatzl wrote:
> Hi,
>
> On Tue, 2016-06-28 at 15:41 -0700, Jon Masamitsu wrote:
>> Thomas,
>>
>> http://cr.openjdk.java.net/~tschatzl/8159978/webrev.1/src/share/vm/gc
>> /g1/g1CollectedHeap.cpp.frames.html
>>
>> For an evacuation failure the region is set to old here
>>
>> 4664       // The region is now considered to be old.
>> 4665       r->set_old();
>>
>> and below the elapsed times are incremented.
>>
>> 4676     if (r->is_young()) {
>> 4677       _young_time += os::elapsedTime() - start_time;
>> 4678     } else {
>> 4679       _non_young_time += os::elapsedTime() - start_time;
>> 4680     }
>>
>> Is it correct that elapsed time for a failed evacuation will be now
>> be added to the _non_young_time?  Whereas before it was included in
>> the _young_time because non_young was assigned a value before the
>> evacuation failure was tested?
> Unintended change. Fixed.

Thanks.
>
> It would have been correct again in the follow-up change that
> parallelizes collection set freeing, so that's why I probably
> overlooked this. Sorry.
No problem.
>
>> [..]
>> http://cr.openjdk.java.net/~tschatzl/8159978/webrev.1/src/share/vm/gc
>> /g1/g1CollectionSet.cpp.frames.html
>>
>>    177 void G1CollectionSet::iterate_from(HeapRegionClosure* cl, uint
>> worker_id, uint total_workers, bool may_be_aborted) {
>>
>> I don't understand this iterate method.  It seems like because of
>> line 196
>>
>>   195     if (cur_pos == len) {
>>   196       cur_pos = 0;
>>   197     }
>>
>> all the workers will process region 0 in the collection set.  Is this
>> method creating slices for each worker?  Or trying to stride through
>> the collection set with each worker getting a different starting
>> index?
> The intent is to let different threads iterate over the entire list
> with each starting at a different index.

Ok.

>
> I updated the change at
> http://cr.openjdk.java.net/~tschatzl/8159978/webrev.2/ (full)
> http://cr.openjdk.java.net/~tschatzl/8159978/webrev.1_to_2/ (diff)
>
> Changes include (from Erik Helin):
>
> - rename G1CollectionSet::set_max_size() to
> G1CollectionSet::initialize() because generally the post-construction
> method in our code base typically is called that.
>
> - removed the may_be_aborted parameter
> from G1CollectionSet::iterate_from(), as it was basically used only for
> paranoia checking code.
>
> - closurified the G1CollectionSet::verify_young_ages()
> and G1CollectionSet::verify_young_cset_indices() methods.
>
> - some comments about MT safeness in g1CollectionSet.hpp
>
> - constified the G1CollectionSet::iterate* methods
>
> - fixed timing attribution

Looks good.

Jon

>
> Testing:
> jprt
>
> Thanks,
>    Thomas
>