RFR (S): 8227442: Make young_index_in_cset zero-based
sangheon.kim at oracle.com
sangheon.kim at oracle.com
Tue Aug 20 04:14:34 UTC 2019
Hi Thomas,
On 8/19/19 6:46 AM, Thomas Schatzl wrote:
> 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)
webrev.1 looks good.
One minor nit:
src/hotspot/share/gc/g1/heapRegion.hpp
618 assert( _age_index == -1, "pre-condition");
- Still there is a whitespace before '_age_index'. Please remove it
before the push.
Thanks,
Sangheon
>
> Passes hs-tier1-3
>
> Thanks,
> Thomas
More information about the hotspot-gc-dev
mailing list