RFR: 8344665: Refactor PartialArrayState allocation for reuse [v2]
Kim Barrett
kbarrett at openjdk.org
Fri Nov 29 19:20:40 UTC 2024
On Thu, 28 Nov 2024 16:22:26 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:
>> I feel the motivation for this encoding is missing from this PR. It's not immediately clear why these two counters need to be combined in this way. Kim outlined some rationale during our offline discussion, but for the benefit of other reviewers and future readers of this code, this rationale should be documented alongside the encoding. Having those arguments written down would help assess whether the additional complexity is justified.
>
> What about using a union/struct instead of all that manual masking and shifting?
>
> I agree that encoding them in this way should give a rationale for that.
I tried using some combination of union/struct a couple of different ways, but
the the result seemd significantly worse than what I published. But some
simplifications since those attempts, plus an idea for a different way of
structuring things, and I've now got something union/struct-based that seems
better.
It could be slightly further improved in syntax if I was willing to drop the
CounterState::_cf member name and make it an anonymous struct. That's a C11
feature that is provided as a C++ extension by all of our supported compilers.
You might not want to look at the difference between the two commits very
much, but instead look at the updated complete change.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22287#discussion_r1863876496
More information about the hotspot-gc-dev
mailing list