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

Stefan Johansson stefan.johansson at oracle.com
Mon Mar 19 15:39:48 UTC 2018

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 (!is_live(_current)) {

I would like to invert the if-statement in the LiveObjIterator constructor and move the call to move_if_below_ntams() into the if-statement updating _current, something like this: if (is_live(_current)) {   // Non-objArrays were scanned by the previous part of that region.   if (_current < mr.start() && !oop(_current)->is_objArray()) {       _current += oop(_current)->size();       // We might have positioned _current on a non-live object. Reposition to the next       // live one if needed.       move_if_below_ntams();     }   } } 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 top_at_rebuild_start = _cm->top_at_rebuild_start(region_idx); 931       assert(!hr->is_old() ||

What do you think about making this assert into a guarantee or use DEBUG_ONLY() to avoid the #ifdef ASSERT? Also maybe adding something to the comment explaining that it is the check on top_at_rebuild_start that filters out eagerly reclaimed regions.

src/hotspot/share/gc/g1/g1_globals.hpp 259   experimental(size_t, 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.

Otherwise I think this looks great.

Thanks, Stefan

Webrevs: http://cr.openjdk.java.net/~tschatzl/8197932/webrev.1to2/ (diff) http://cr.openjdk.java.net/~tschatzl/8197932/webrev.2/ (full) testing: hs-tier1-5 Thanks, Thomas

More information about the hotspot-gc-dev mailing list