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

[Thomas Schatzl]

Hi Kim,

  thanks for reviewing.

On Thu, 2019-05-09 at 20:49 -0400, Kim Barrett wrote:
> > On May 6, 2019, at 5:51 AM, Thomas Schatzl <
> > thomas.schatzl at oracle.com> wrote:
> > 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
> 
> =====================================================================
> =========
> http://cr.openjdk.java.net/~tschatzl/8200545/webrev/

Sorry for any confusion I caused with this change.

> 
> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/g1/g1InCSetState.hpp
>   58   // The first two values are encoded in increasing generation
> order, which
>   59   // allows us to use them as index into arrays; they also start
> at zero.
> 
> This part of the comment doesn't seem true.  Unless NotInCSet (value
> == 0) is considered a "generation", but then there are 3 rather than
> 2.  The comment formerly talked about the "positive values" (e.g. the
> values > 0).
> 

Fixed in .2.

> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/g1/g1InCSetState.hpp
>   67   static const in_cset_state_t NumGenInCSet =   3;    // Number
> of generations that can be in the collection set.
> 
> Again here, confusion about numbering.  There are only 3 generations
> if NotInCSet is considered a "generation".
> 

Yes, this is a misnomer. The G1HeapRegionAttr contents are
copies/shortcuts for often used information during GC only.

I reverted it to just "Num" as before.

> And places that use these as array indices generally "waste" an entry
> for NotInCSet.  I thought there was a CR for that, but couldn't find
> it.

This had at least initially been intentional. I do not remember a CR
for that.

When originally introducing this table, I deliberately wanted to make
the decision between in cset and not in cset a decision simple.
Initially this has just been != 0 / == 0, but with the proliferation of
information in that entry (first humongous reclaim candidate, then
optional evacuation) it has changed to > 0.

You are right that we might want to reconsider this decision now. It's
not a big waste of space in the arrays. I filed JDK-8223693 for
cleaning this up.

> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/g1/g1InCSetState.hpp
>   46 #ifdef SPARC
>   47   uint32_t _needs_remset_update;
>   48 #else
>   49   uint8_t _needs_remset_update;
>   50 #endif
> 
> Is it actually worth conditionalizing the type of this "bool" member?

This has changed in .2; I conditionalized the entries to get minimum
size of the element (i.e. sizeof(G1HeapRegionAttr)) for non-sparc, and
word size for sparc for best performance.

Note that I did not check SPARC performance, but I can see advantages
to keep memory footprint low(er) for that table on everything else.
(SPARC implementations I have tested are comparatively slow extracting
parts of a word).

> Note that access via needs_remset_update() is going to be doing an
> integral to bool conversion.

I fixed the implicit integral to bool conversion, that's indeed an
unintentional mistake.

> -------------------------------------------------------------------
> -----------
> 
> I kind of wish some of the changes to g1InCSetState.hpp had been done
> separately.  Oh well...

What exactly do you suggest? Separate renaming from the functional
change? Or first clean up InCSetState (typedefs, enum, JDK-8223693,
..), then rename and then add the new functionality?

> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/g1/g1OopClosures.hpp
>   65   G1ScanCardClosure(G1CollectedHeap* g1h,
>   66                                   G1ParScanThreadState* pss) :
> 
> Fix indentation on line 66.

Fixed in .2.

> 
> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/g1/heapRegionRemSet.hpp
>  242 //      log_error(gc)("Region %u set complete", _hr-
> >hrm_index());
> 
> Some left-over debugging code?

Fixed in .2.

> 
> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/g1/g1CollectedHeap.cpp
> 2629 class RegisterHumongousWithInCSetFastTestClosure : public
> HeapRegionClosure {
> 
> This class seems misnamed now, since it is now doing stuff for all
> regions (calls register_region_with_cset for non-humongous).  (Maybe
> this is addressed in one of the other associated webrevs?)
> 
> Similarly, record_fast_reclaim_humongous_stats() seems somewhat
> misnamed now, since the time value is for "register region".

Fixed.

New webrevs:


http://cr.openjdk.java.net/~tschatzl/8200545/webrev.2_t o_3/ (diff)

http://cr.openjdk.java.net/~tschatzl/8200545/webrev.3/ (full)

testing: local compilation

Thanks,
  Thomas