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