RFR (7xS): 8177044: Remove _scan_top from HeapRegion

[Erik Helin]

On 05/31/2017 10:19 AM, Thomas Schatzl wrote:
> Hi all,

Hi Thomas!

>   Erik had a look at these changes and had minor comments:
>
> I.e. he asked about not removing one default value for a parameter in
> the declaration of the constructor of G1UpdateRSOrPushRefOopClosure
> (and do it later).
>
> Also, one call to memset() is redundant and has been removed.
>
> Here are the changes again:
>
> http://cr.openjdk.java.net/~tschatzl/8177044/webrev.1_to_2/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8177044/webrev.2/ (full)

Looks good to me, thanks for cleaning this up!
Erik

> Thanks,
>   thomas
>
> On Thu, 2017-05-11 at 16:18 +0200, Thomas Schatzl wrote:
>> Hi Kim and Sangheon,
>>
>> On Tue, 2017-05-09 at 11:12 -0700, sangheon wrote:
>>>
>>> Hi Thomas,
>>>
>>> On 05/05/2017 05:13 AM, Thomas Schatzl wrote:
>>>>
>>>>
>>>> Hi all,
>>>>
>>>>    recent reviews have made changes necessary to parts of the
>>>> changeset
>>>> chain.
>>>>
>>>> Here is a list of links to updated webrevs. Since they have
>>>> apparently
>>>> not been reviewed yet, I simply overwrote the old webrevs.
>>>>
>>>> JDK-8177044: Remove _scan_top from HeapRegion
>>>> http://cr.openjdk.java.net/~tschatzl/8177044/webrev/
>>> Looks good to me.
>>> And agree to Kim about retaining the comment.
>>>
>>> src/share/vm/gc/g1/g1RemSet.cpp
>>>   765   if (scan_limit <= start) {
>>>   766     // If the trimmed region is empty, the card must be
>>> stale.
>>>   767     return false;
>>>
>>   thanks for your review.
>>
>> For reference, here is the comment I intend to push:
>>
>> --- old/src/share/vm/gc/g1/g1RemSet.cpp	2017-05-11
>> 16:14:56.054517736 +0200
>> +++ new/src/share/vm/gc/g1/g1RemSet.cpp	2017-05-11
>> 16:14:55.951514554 +0200
>> @@ -735,6 +735,7 @@
>>
>>    HeapWord* scan_limit = _scan_state->scan_top(r->hrm_index());
>>    if (scan_limit <= start) {
>> +    // If the card starts above the area in the region containing
>> objects to scan, skip it.
>>      return false;
>>    }
>>
>> because the original comment is wrong now.
>>
>> New Webrevs:
>> http://cr.openjdk.java.net/~tschatzl/8177044/webrev.0_to_1/ (diff)
>> http://cr.openjdk.java.net/~tschatzl/8177044/webrev.1/ (full)
>>
>> Thanks again for your reviews,
>>   Thomas
>>