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

[Thomas Schatzl]

Hi,

On Fri, 2019-05-10 at 18:53 -0400, Kim Barrett wrote:
> > 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.

I removed the comment; it does not seem necessary, and does not answer
any real question from what I can see.

I assigned JDK-8223693 to me and will fix that asap to remove the
problem with the NotInCSet value.

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

It's like three or four words per gc thread during evacuation afaict.

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

We want to have control over the size of the G1HeapRegionAttr data
structure to minimize its size due to already being very cache/memory
bound during evacuation. So the intent is to not exacerbate the issue
further by increasing the size of this table more than necessary. SPARC
is the exception due to its very large performance penalty when
accessing sub-word memory locations and/or the compiler doing really
weird things.

Initially the webrev used the LSB of the existing value member, i.e. no
separate "needs_remset_update" member for this reason. I backed away
from doing so before posting the webrev for clarity, and I intend to
add information in the future anyway to require 16 bits. Some perf
tests comparing 8 to 16 bits element entries did not show a measurable
difference.

[Background/anecdotal evidence:

- During initial introduction of InCSetState I wrote "[...] for SPARC
we define _value as intptr_t instead of int8_t. This avoids the
mentioned shifts at the cost of some memory. However we want to use
int8_t on other architectures because tests indicated that it is
(slightly) faster. [...]"


http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2014-December/011557.html

- some a kind of G1 clone for some different use case I worked on in
the 2008-2011 timeframe also had a similar (16 bit entry; with some
more fields using C-bitfields) table for the same reasons. I remember
doing a lot of measurements until being satisfied and convinced that it
is worth the complexity.]

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

There has been such a separation for the initial webrev (the "webrev"
vs. the "webrev.rename" webrevs), but I did not continue that further
down on assuming that after that the renaming would be accepted and I
was not required to continue providing two webrevs for each.

I thought this would be enough, but it looks like I erred here. I will
separate these changes out more obviously in the future. Apologies.

New webrevs:

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

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

Thanks,
  Thomas