RFR [2/2] 8065258: JDK-8065358 Refactor G1s usage of save_marks and reduce related races

[Mikael Gerdin]

Hi Bengt,

thanks for taking a look at this and for the discussions leading to me 
developing this change.

On 2014-11-20 11:01, Bengt Rutisson wrote:
>
> Hi Mikael,
>
> This looks good to me.
>
> Two minor comments in G1OffsetTableContigSpace::record_timestamp():
>
> The register volatile keywords here look like a remainder from the
> debugging of the original top() reordering problem.
>
> 1027     register HeapWord* volatile st = _scan_top;

Yes, I'll remove those.
Thanks for catching this.

>
>
> This check:
>
> 1021   if (_gc_time_stamp < curr_gc_time_stamp) {
>
> Was already there in the old code but it looks odd to me. We should
> never have _gc_time_stamp > curr_gc_time_stamp and if they are the same
> it doesn't hurt to set it. So, I guess this is an unnecessary check. Can
> we turn it into a guarantee(_gc_time_stamp <= curr_gc_time_stamp)
> instead? Maybe that should be done as a separate change...

I agree that this looks odd. I'd prefer to defer this to a future change.

/Mikael

>
> Thanks,
> Bengt
>
>
>
> On 2014-11-20 09:46, Mikael Gerdin wrote:
>> Hi all,
>>
>> This is the second patch of two to fix JDK-8065358
>>
>> In this change I suggest that we introduce a new field _scan_top to
>> G1OffsetTableContigSpace to keep track of the maximum address where a
>> GC worker thread is allowed to scan objects in old regions which have
>> been used as allocation regions during the current evacuation cycle.
>>
>> More background (from the bug):
>> G1 currently uses the _saved_mark_word field in Space to detemine a
>> point in a region above which a current GC cannot scan objects. This
>> is needed to prevent card scanning threads from scanning in memory
>> regions where other gc workers are concurrently allocating objects.
>>
>> This re-use of the _saved_mark_word field causes confusion to readers
>> of the code since it's not used for the same purpose as the other
>> Space classes.
>> The setting and reading of this field, and the per region gc time
>> stamp which accompanies it is also unnecessarily racy. Previously the
>> _saved_mark_word field was set to the value of the _top pointer when a
>> region was selected as a GC allocation region and the time stamp was
>> set to indicate that the saved mark field should be used as a maximum
>> address. This code had some problems with races in JDK-8058209 and
>> could be redesigned in a less racy way.
>>
>> Suggested fix is to introduce a new field in HeapRegions to keep track
>> of the maximum address where card scanning is allowed and set that
>> field at the point of retaining an OldGCAllocRegion instead of when
>> allocations are being started. That way we get rid of the store
>> ordering requirement in the timestamp record path.
>>
>> There is still a race between the per region time stamps and the per
>> region top pointer, where we must ensure that the time stamp store
>> must be visible before any subsequent top stores.
>> This store ordering is enforced by the fact that all stores to top are
>> either ordered with #storestore (the initial allocation) or guarded by
>> a Mutex. To ensure that the reader path sees a consistent view it must
>> be exectued with the proper load ordering, where we must read top
>> before the time stamp in order to ensure that we don't see a top value
>> which has been updated by a concurrent gc worker if we see a time
>> stamp from a previous gc cycle.
>>
>> Webrev: http://cr.openjdk.java.net/~mgerdin/8065358/webrev.0/
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8065358
>> Testing: JPRT, Kitchensink (4hr+)
>>
>> Thanks
>> /Mikael
>