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

[Thomas Schatzl]

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)

Thanks,
  Thomas