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

[Mikael Gerdin]

Hi,

(I just noticed that I mistyped the bug number in the subject, it should 
be 8065358 twice.)

I removed the weird qualifier combination from "st" in record_timestamp().

Thomas also suggested to me that the guarantee in record_retained_region 
may in fact be incorrect in some corner cases, I removed it since no 
other part of the code actually depends on this being true.

New webrev at:
http://cr.openjdk.java.net/~mgerdin/8065358/webrev.1/
Incremental webrev at:
http://cr.openjdk.java.net/~mgerdin/8065358/webrev.0_to_1/

/Mikael

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