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

[Stefan Johansson]

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()?

  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?

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.
---

Thanks,
Stefan


> Testing:
> local testing with existing tests
> (gc/g1/TestEagerReclaimHumongousRegionsClearMarkBits.java)
>
> Thanks,
>    Thomas