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

[Thomas Schatzl]

Hi Kim,

   thanks for your review.

On 17.08.19 06:48, Kim Barrett wrote:
>> 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?

Done.

> 
> ------------------------------------------------------------------------------
> 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.

Removed.

> 
> ------------------------------------------------------------------------------
> 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.

Removed the first occurrence.

Sorry for these issues, which stem from me moving around this change in 
my patch queue :(

http://cr.openjdk.java.net/~tschatzl/8227442/webrev.0_to_1/ (diff)
http://cr.openjdk.java.net/~tschatzl/8227442/webrev.1/ (full)

Passes hs-tier1-3

Thanks,
   Thomas