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

[Bengt Rutisson]

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;


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

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