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

[Thomas Schatzl]

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.

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

(Test: local compilation)

Thanks,
  Thomas