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

sangheon.kim sangheon.kim at oracle.com
Tue Mar 27 17:50:45 UTC 2018

Hi Thomas,

Finally the last piece! :)

On 03/22/2018 03:46 AM, Thomas Schatzl wrote:

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.2to3 (diff) http://cr.openjdk.java.net/~tschatzl/8197932/webrev.3 (full) webrev.3 looks good! Minor nits below.

============================= src/hotspot/share/gc/g1/g1RemSet.cpp  805       void move_to_next() {  810       oop next() const { (Ignore if you don't agree)

============================= src/hotspot/share/gc/g1/g1RemSetTrackingPolicy.cpp   37   // All non-young and non-closed archive regions need to be scanned for references;   38   // At every gc we gather references to other regions in young, and closed archive   39   // regions by definition do not have references going outside the closed archive.

============================= src/hotspot/share/gc/g1/g1_globals.hpp  261           range(4 * K, 32 * M)                                              \

Thanks, Sangheon

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