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

[Bengt Rutisson]

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.

Thanks,
Bengt



>>
>> What do you think about asserting that the bitmap is clear in
>> ConcurrentMarkThread::run() if cm()->has_aborted() ?
>    good thoughts.
>
> I updated the change and JDK-7098512, new webrev at
> http://cr.openjdk.java.net/~tschatzl/8048085/webrev.1/
>
> Diff:
> http://cr.openjdk.java.net/~tschatzl/8048085/webrev.0_to_1/
>
> Testing:
> jprt
>
> Thanks,
>    Thomas
>
>