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

[Bengt Rutisson]

Hi Thomas,

Looks good.

Thanks,
Bengt


On 2014-07-14 09:57, Thomas Schatzl wrote:
> 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
>