RFR (XXS): 8048085: Aborting marking just before remark results in useless additional clearing of the next mark bitmap

[Thomas Schatzl]

Hi Bengt,

On Thu, 2014-07-10 at 11:49 +0200, Bengt Rutisson wrote:
> Hi Thomas,
> 
> On 2014-07-10 11:38, Thomas Schatzl wrote:
> > Hi Bengt,
> >
> >    thanks for the review.
> >
> > On Mon, 2014-07-07 at 15:44 +0200, Bengt Rutisson wrote:
> >> Hi Thomas,
> >>
> >> On 2014-07-07 14:46, Thomas Schatzl wrote:
> >>> Hi all,
> >>>
> >>>     can I have reviews for the following minor performance fix noticed
> >>> during work on JDK-8038423 (G1: Decommit memory within the heap)?
> >>>
> >>> The situation is that when a full GC aborts concurrent marking, the
> >>> concurrent mark thread needlessly clears the next mark bitmap again
> >>> (concurrently this time) in ConcurrentMark::abort().
> >>>
> >>> The fix is to skip this phase in this case.
> >>>
> >>> An alternative, always keeping this phase but not doing this bitmap
> >>> clear at Full GC abort (JDK-7098512) would be possible. However doing
> >>> that would make JDK-8048084 (sending out reviews soon) much harder as we
> >>> would need to keep track of regions that have become unavailable.
> >>>
> >>> This change also does not change anything about duration of Full GC. So
> >>> I would prefer to postpone JDK-7098512.
> >>>
> >>> Webrev:
> >>> http://cr.openjdk.java.net/~tschatzl/8048085/webrev/
> >> Looks good.
> >>
> >> A couple of minor comments:
> >>
> >> The comment in ConcurrentMark::abort() for the bitmap clearing was
> >> always misleading (as stated in JDK-7098512). Would maybe be good to
> >> update it now to explain why it is really needed.
> 
> You added a call to:
> 
> g1h->check_bitmaps("Reset bitmaps");
> 
> I think that only checks that there are no bits set over TAMS. But you 
> can actually check that he whole bitmap is empty. I was thinking 
> something like (completely untested code):
> 
> assert(_nextMarkBitMap->getNextMarkedWordAddress(_nextMarkBitMap->startWord()) 
> == _nextMarkBitMap->endWord(), "Next bitmap has not been cleared");
> 
> Also, check_bitmaps() only hits if G1VerifyBitmaps. Maybe that is good. 
> In that case I guess the assert I suggest should be guarded by that too.

Okay, sorry. All fixed I think.

New webrev:
http://cr.openjdk.java.net/~tschatzl/8048085/webrev.2

Diff:
http://cr.openjdk.java.net/~tschatzl/8048085/webrev.1_to_2/

Testing:
jprt, aurora-adhoc with VerifyBitmaps enabled

Thanks,
Thomas