RFR (M): 8200545: Improve filter for enqueued deferred cards

[sangheon.kim at oracle.com]

Hi Thomas,

On 5/6/19 2:51 AM, Thomas Schatzl wrote:
> Hi Sangheon,
>
>    thanks for your review.
>
> On Tue, 2019-04-30 at 13:38 -0700, sangheon.kim at oracle.com wrote:
>> Hi Thomas,
>>
>> On 4/25/19 12:33 AM, Thomas Schatzl wrote:
>>> Hi all,
>>>
>>>     ErikD had some improvement suggestions during an internal
>>> review:
>>>
>>>
>>> [...]
>>> http://cr.openjdk.java.net/~tschatzl/8200545/webrev.0_to_1/
>>> http://cr.openjdk.java.net/~tschatzl/8200545/webrev.1/
>> Looks good.
>> I have some minor nits below:
>>
>> -------------------------------------------------------------------
>> ----
>> src/hotspot/share/gc/g1/g1CollectedHeap.cpp
>> 3330              "Only allowed InCSet state is IsHumongous, but is
>> %d", region_attr.type());
>> - Should be G1HeapRegionAttr not InCSet.
> Fixed.
>
>> -------------------------------------------------------------------
>> ----
>> src/hotspot/share/gc/g1/g1CollectedHeap.hpp
>>    569   inline void register_humongous_region_with_region_attr(uint
>> index);
>>    578   inline void register_region_with_region_attr(HeapRegion* r);
>>    579   inline void register_old_region_with_region_attr(HeapRegion*
>> r);
>>    580   inline void
>> register_optional_region_with_region_attr(HeapRegion* r);
>> - (Just a suggestion) Only 569 has index parameter but looking at
>> the caller sites of other 3 methods,
>> only register_optional_region_with_region_attr() doesn't use
>> hrm_index at the caller site. So we can consider using hrm index as a
>> parameter.
> It was not that clear cut to me to use one or the other, and still is
> not. I will defer that issue for a different CR.
Okay.

>
>> -------------------------------------------------------------------
>> ----
>> src/hotspot/share/gc/g1/g1CollectedHeap.inline.hpp
>>    175 }
>>    176 void
>> G1CollectedHeap::register_old_region_with_region_attr(HeapRegion* r)
>> {
>> - Need new line between 175 and 176.
>>
>> -------------------------------------------------------------------
>> ----
>> src/hotspot/share/gc/g1/g1OopClosures.hpp
>>     65   G1ScanCardClosure(G1CollectedHeap* g1h,
>>     66                                   G1ParScanThreadState* pss) :
>> - Wrong alignment between 65 and 66.
>>
>> -------------------------------------------------------------------
>> ----
>> src/hotspot/share/gc/g1/g1ParScanThreadState.hpp
>>     55   AgeTable          _age_table;
>>     56   G1HeapRegionAttr       _dest[G1HeapRegionAttr::NumGenInCSet];
>> - Wrong alignment at line 56.
> Fixed. For the last one, I removed the alignment that was not
> completely applied anyway completely.
>
> http://cr.openjdk.java.net/~tschatzl/8200545/webrev.1_to_2/
> http://cr.openjdk.java.net/~tschatzl/8200545/webrev.2/
Looks good to me.

Thanks,
Sangheon


> (Test: local compilation)
>
> Thanks,
>    Thomas
>
>