RFR [M][7/7]: 8197932: Better split work in rebuild remembered sets phase (original) (raw)

Thomas Schatzl thomas.schatzl at oracle.com
Thu Mar 22 10:46:02 UTC 2018

Hi Stefan,

thanks for your review, and sorry for the late reply...

On Mon, 2018-03-19 at 16:39 +0100, Stefan Johansson wrote:

Thanks Thomas,

On 2018-03-19 13:03, Thomas Schatzl wrote: > Hi all, > > Stefan proposed to use some iterator over the work area that > internally handles the boundary at the TAMS wrt to liveness when > scanning. > > This makes the change a bit more compact, performance does not seem > to differ, so why not... :) I really like this approach :) A few additional small things: src/hotspot/share/gc/g1/g1RemSet.cpp 787 if (!islive(current)) { I would like to invert the if-statement in the LiveObjIterator constructor and move the call to moveifbelowntams() into the if-statement updating current, something like this: if (islive(current)) { // Non-objArrays were scanned by the previous part of that region. if (current < mr.start() && !oop(current)->isobjArray()) { current += oop(current)->size(); // We might have positioned current on a non-live object. Reposition to the next // live one if needed. moveifbelowntams(); } } } else { ... } 926 #ifdef ASSERT 927 // In the final iteration of the loop the region might have been eagerly reclaimed. 928 // Simply filter out those regions. We can not just use region type because there 929 // might have already been new allocations into these regions. 930 HeapWord* const topatrebuildstart = cm->topatrebuildstart(regionidx); 931 assert(!hr->isold() || What do you think about making this assert into a guarantee or use DEBUGONLY() to avoid the #ifdef ASSERT? Also maybe adding something to the comment explaining that it is the check on topatrebuildstart that filters out eagerly reclaimed regions.

Done.

--- src/hotspot/share/gc/g1/g1globals.hpp 259 experimental(sizet, G1RebuildRemSetChunkSize, 256 * K,

Did you test to see if changing the G1RebuildRemSetChunkSize flag had any impact on performance, otherwise maybe we could make it a constant instead.

The problem is not so much increasing the value, but decreasing it for very slow systems. Decreasing it further adds some overhead.

The other point is that since we track the time a chunk takes, it seems strange to not have any knobs to deal with problems in that area.

New webrev: http://cr.openjdk.java.net/~tschatzl/8197932/webrev.2_to_3 (diff) http://cr.openjdk.java.net/~tschatzl/8197932/webrev.3 (full)

In this webrev I also did some somewhat messy looking renaming of nTAMS to TAMS (removing the n) and passing the (n)TAMS around in conjunction with the bitmap. This will make a follow-up change a lot easier to read.

Thanks, Thomas

More information about the hotspot-gc-dev mailing list