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

[sangheon.kim at oracle.com]

Hi Thomas,

On 4/25/19 12:33 AM, Thomas Schatzl wrote:
> Hi all,
>
>    ErikD had some improvement suggestions during an internal review:
>
> - he suggested to introduce a getter that returns you a string for the
> region type. This allows removal of the *FORMAT defines and generally
> looks better.
>
> - also added typedefs for both members of the G1HeapRegionAttr for
> uniformity.
>
> - removed some confusing comments, and some debug code.
>
> The changes are based on the "full" version further below. So it is
> probably still useful to look at the "webrev" changes first to review
> the important differences.
>
> 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.

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

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

Thanks,
Sangheon


>
> Thanks,
>    Thomas
>
> On Mon, 2019-04-15 at 09:29 +0200, Thomas Schatzl wrote:
>> Hi all,
>>
>>    can I have reviews for this change that improves the check whether
>> the destination region for a reference has a remembered set that
>> needs updating?
>>
>> This removes the need to use two distinct closures for scanning oops
>> during evacuation, as both the new and the old variant are now equal
>> in performance, so not only simplifying existing code, but also
>> simplifying the follow-up change JDK-8213108 a lot.
>>
>> The idea of this change is to put the information whether a region
>> needs remembered set updates into the InCSetState table. This results
>> in that for every reference we do not need to (re-)calculate the
>> destination region, and then do the walk across several data
>> structures (HeapRegion->HeapRegionRemSet) every time - this simply
>> combines this with the existing calculation of InCSetState.
>>
>> The code adds a new member to InCSetState that indicates whether the
>> region needs remembered set updates.
>>
>> As a second, seperate step, there is a change that renames
>> InCSetState in a imho more understandable G1RegionAttr(ibutes); if
>> wanted I can do this in a separate CR, but here I will just present
>> them as separate webrevs.
>>
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8200545
>> Webrev:
>> http://cr.openjdk.java.net/~tschatzl/8200545/webrev/ (add InCSetState
>> member only)
>> http://cr.openjdk.java.net/~tschatzl/8200545/webrev.rename/ (rename
>> InCSetstate only)
>> http://cr.openjdk.java.net/~tschatzl/8200545/webrev.full/ (full
>> change)
>> Testing:
>> hs-tier1-5, internal perf testing;
>>
>> While testing I found JDK-8222426 - however that one also occurs
>> without the changes, so I think we can go ahead with this regardless.
>>
>> Thanks,
>>    Thomas
>>
>>
>>
>>
>