RFR (S): 8227442: Make young_index_in_cset zero-based

Kim Barrett kim.barrett at oracle.com
Sat Aug 17 04:48:55 UTC 2019


> On Aug 7, 2019, at 6:39 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> Hi all,
> 
>  can I have reviews for this refactoring that changes the minimum index for the young indices (used for determining survivors per young region) from -1 to 0?
> 
> This avoids some imho unnecessary increment in the copy_to_survivor_space() method.
> 
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8227442
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8227442/webrev/
> Testing:
> hs-tier1-5 almost done with no issues
> 
> Thanks,
>  Thomas

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/heapRegion.hpp
 578     assert(_surv_rate_group != NULL, "pre-condition" );
and several others

If you are going to the bother of removing leading whitespace from
these asserts, why not also do the trailing whitespace?

------------------------------------------------------------------------------ 
src/hotspot/share/gc/g1/heapRegion.hpp
 572     assert(index != 0, "just checking");
 573     assert((index == 0) || is_young(), "pre-condition" );

`index == 0` check on 573 is not useful with the preceeding check.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1ParScanThreadState.cpp

In G1ParScanThreadState::copy_to_survivor_space:

[existing and retained, though modified for +1 removal]
 225   HeapRegion* const from_region = _g1h->heap_region_containing(old);
 226 
 227   const int young_index = from_region->young_index_in_cset();

[added later in the same function]
 280     HeapRegion* const from_region = _g1h->heap_region_containing(old);
 281     const uint young_index = from_region->young_index_in_cset();

I'm assuming the new addition was to put young_index closer to the
scope where it is actually used?  I think the earlier declaration is
now unused except by the immediately following assert.

If it's important to have that assert up front, the associated
from_region and young_index ought to be debug-only too.

I'd prefer not having these nested declarations for the same variables.

------------------------------------------------------------------------------




More information about the hotspot-gc-dev mailing list