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

[Jon Masamitsu]

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?

Old code is


4811 if (non_young) {
4812 non_young_time_ms += elapsed_ms;
4813 } else {
4814 young_time_ms += elapsed_ms;
4815 }


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?

Jon


On 6/28/2016 5:19 AM, Thomas Schatzl wrote:
> Hi all,
>
> On Mon, 2016-06-27 at 14:19 +0200, Thomas Schatzl wrote:
>>   Hi all,
>>   
>>     can I have reviews for the following change that changes the
>>   collection set to use an external array of region ids instead of
>>   linking them through the HeapRegion class.
>>   
>    I got some feedback from Erik Helin about this change. These are
>
> - we do not really need the multiple-writer/multiple-reader
> implementation to store the list of collection set regions. We only
> ever have one concurrent reader and one concurrent writer to it.
>
> So I simplified the data structure to use only storestore/loadload
> barriers for synchronization. The relevant changes got so small that an
> extra data structure was not warranted, so I removed it.
>
> - I strengthened (added) some asserts about some methods that should
> only be called by the vm thread at a safepoint to support this. Just to
> indicate that in these methods we do not need the memory barriers.
>
> - fixed a bug in the collection set iteration where the
> HeapRegionClosure::incomplete() method has not been called if the
> iteration was aborted.
>
> - some cleanup
>
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8159978/webrev.1 (full)
> http://cr.openjdk.java.net/~tschatzl/8159978/webrev.0_to_1/ (diff
>
> Testing:
> jprt
>
> Thanks,
>    Thomas
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160628/bb0c0877/attachment.htm>