On Fri, 26 Jul 2024 18:39:27 GMT, Vladimir Ivanov <vliva...@openjdk.org> wrote:
> Oh, it comes as a surprise to me... I was under impression that the first > thing hand-coded assembly variants do is check for `bitmap != > SECONDARY_SUPERS_BITMAP_FULL`. At least, it was my recollection from working > on [JDK-8180450](https://bugs.openjdk.org/browse/JDK-8180450). (And the > initial version of the patch with the check in > `Klass::lookup_secondary_supers_table()` and > `_bitmap(SECONDARY_SUPERS_BITMAP_FULL)` only reassured me that's still the > case.) > > > The root cause of this bug is that we're not maintaining the invariant > > popcount(bitmap) == secondary_supers()->length(). > > The invariant holds only when `bitmap != SECONDARY_SUPERS_BITMAP_FULL`. Yes, exactly so. That's what I intended to mean. > It does help that even in case of non-hashed `secondary_supers` list > `secondary_supers()->length() >= popcount(bitmap)`, but initial probing > becomes much less efficient (a random probe followed by a full linear pass > over secondary supers list). > > Alternatively, all table lookups can be adjusted to start with `bitmap != > SECONDARY_SUPERS_BITMAP_FULL` checks before probing the table. It does add a > branch on the fast path (and slightly increases inlined snippet code size), > but the branch is highly predictable and works on a value we need anyway. True enough. I've been trying to move as much as I can out of the inlined code, though. > So, I would be surprised to see any performance effects from it. IMO it's > easier to reason and more flexible: `SECONDARY_SUPERS_BITMAP_FULL == bitmap` > simply disables all table lookups and unconditionally falls back to linear > search. I take your point. But that seems like it's a bit of a sledgehammer to crack a walnut, don't you think? Given that it's only necessary to fix a rare edge case. But I'm not going to be precious about this choice, I'll test for a full bitmap first if you prefer. It's only a couple of instructions, but they are in the fast path that is successful in almost 99% of cases. ------------- PR Comment: https://git.openjdk.org/jdk/pull/19989#issuecomment-2253537338
