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

[Bengt Rutisson]

Inline far below...

On 11/20/14 11:24 AM, Mikael Gerdin wrote:
> 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.

Just to close the loop on this. I'm fine with deferring this. Can you 
file a bug for it?

Thanks,
Bengt

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