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

Kim Barrett kim.barrett at oracle.com
Mon May 13 22:30:21 UTC 2019


On May 13, 2019, at 5:48 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> On Fri, 2019-05-10 at 18:53 -0400, Kim Barrett wrote:
>>>> 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.

Right, which seems like not a lot.  I think the main issue is the chance
of confusion; not sure it’s worth the development effort (esp. perf.
testing) needed to change it at this point.

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

I'm curious where that's an issue, but we can talk about it offline.
I also wonder whether "old" performance measurements might have been
affected / spoiled by old value passing ABI, since the state type is
not POD but is trivially copyable / destructable.

Note that sizeof(bool) is typically 1 (though the standard explicitly
does not guarantee that).  But if there are going to be more bits
coming, then okay.

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

I joined the discussion after revisions had been made, and started reviewing
with webrev.2.  In conjunction with how updates were provided, that led to trouble…

> New webrevs:
> 
> http://cr.openjdk.java.net/~tschatzl/8200545/webrev.3_to_4/ (diff)
> 
> http://cr.openjdk.java.net/~tschatzl/8200545/webrev.4/ (full)

Looks good.




More information about the hotspot-gc-dev mailing list