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

Stefan Johansson stefan.johansson at oracle.com
Thu Mar 29 12:25:23 UTC 2018

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 maybeclearbitmapifset(G1CMBitMap* bitmap, HeapWord* addr) It will always clear if set, so I think we should rename it. What do you think about clearbitmapifmarked() or clearmarkinbitmap()? I changed it to clearmarkifset. 544 G1CollectorState* collectorstate = g1h->collectorstate(); 545 if (collectorstate->markorrebuildinprogress() || 546 collectorstate->clearingnextbitmap()) { 547 maybeclearbitmapifset(nextmarkbitmap, 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 clearstatistics even if markorrebuildinprogress() 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 markorrebuildinprogress() 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

More information about the hotspot-gc-dev mailing list