RFR (S): 8200385: Eagerly reclaimed humongous objects leave mark in prev bitmap

[Stefan Johansson]

Thanks Thomas,

On 2018-03-29 12:55, Thomas Schatzl wrote:
> On Thu, 2018-03-29 at 11:53 +0200, Stefan Johansson wrote:
>> On 2018-03-29 11:12, Thomas Schatzl wrote:
>>> Hi all,
>>>
>>>     can I have reviews for this small fix to a benign bug, that is I
>>> haven't seen any actual product failure from it but some very rare
>>> test failures, where when we eagerly reclaim humongous objects we
>>> leave a mark on the prev bitmap in some cases?
>>>
>>> The suggested fix is to always look at the prev bitmap and clear
>>> it, and if needed also clear potential marks in the next bitmap.
>>>
>>> To make the failure appear basically 100% in that test, I added a
>>> simple assert after reclaiming the humongous object.
>>>
>>> With the fix, this failure goes away completely.
>>>
>>> Note that this is more a "data structure hygiene" fix - the stray
>>> mark on the prev bitmap will be automatically cleared after
>>> switching the bitmaps at cleanup and preparing that bitmap for the
>>> next mark.
>>>
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8200385
>>> Webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8200385/webrev/
>> Good fix, even though the problem is benign it is always nice to have
>> a consistent state. A few comments:
>> src/hotspot/share/gc/g1/g1ConcurrentMark.cpp
>>    532 static void maybe_clear_bitmap_if_set(G1CMBitMap* bitmap,
>> HeapWord* addr)
>>
>> It will always clear if set, so I think we should rename it. What do
>> you think about clear_bitmap_if_marked() or clear_mark_in_bitmap()?
> I changed it to clear_mark_if_set.
>
>>    544   G1CollectorState* collector_state = _g1h->collector_state();
>>    545   if (collector_state->mark_or_rebuild_in_progress() ||
>>    546       collector_state->clearing_next_bitmap()) {
>>    547     maybe_clear_bitmap_if_set(_next_mark_bitmap, r->bottom());
>>    548   }
>>
>> I get that this might be a bit more efficient, but I would prefer
>> always clearing both bitmaps, to not have to depend on the state. Or
>> would there be any problem with that?
> I do not think so. I will change that as suggested.
>
>> There is also a difference here that will now clear_statistics even
>> if mark_or_rebuild_in_progress() returns false. Is this on purpose? I
>> don't see any problem with this just wanted to check.
> Not on purpose, it does not matter. The statistics are not used beyond
> the mark_or_rebuild_in_progress() part. I will revert to the old code.
>
> http://cr.openjdk.java.net/~tschatzl/8200385/webrev.1 (full only; sorry
> I messed up the incremental diff)
No problem, this looks good!
Cheers,
Stefan
> Thanks,
>    Thomas
>