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

[Kim Barrett]

> On May 10, 2019, at 6:18 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> On Thu, 2019-05-09 at 20:49 -0400, Kim Barrett wrote:
>> -------------------------------------------------------------------
>> -----------
>> 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.

Looking at webrev.3 (latest), I still see the above quoted comment that I
think is wrong, or at least very confusing.

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

OK.  Though I agree the space impact is likely small, and it might not be
worth the trouble to fix anymore, despite it being potentially confusing.

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

Why not just make the member type “bool” and be done with it?
Let the compiler decide the appropriate representation.

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

I think separating renamings from functional changes might have made
reviewing easier, at least for me.  Of course, that’s more work for you.

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

Thanks.

> 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