On Thu, 25 Jul 2024 23:31:21 GMT, Vladimir Ivanov <vliva...@openjdk.org> wrote:
> Thanks! The patch looks good, except there was one failure observed during > testing with the latest patch [1]. It does look related to the latest changes > you did in > [54050a5](https://github.com/openjdk/jdk/pull/19989/commits/54050a5c2c0aa1d6a9e36d0240c66345765845e3) > about `bitmap == SECONDARY_SUPERS_BITMAP_FULL` check. Wow! Whoever wrote that test case deserves a bouquet of roses. It's a super-edge case. If the hash slot of an interface is 63, and the secondaries array length of the Klass we're probing is 63, then the initial probe is at Offset 63, one past the array bounds. This bug happens because of an "optimization" created during the first round of reviews. If the secondaries array length is >= 62 (_not_ >= 63), we set the secondaries bitmap to `SECONDARY_SUPERS_BITMAP_FULL`. So, the initial probe sees a full bitmap, popcount returns 63, and we look at secondary_supers[63]. _Bang_. We never noticed this problem before because there's no bounds checking in the hand-coded assembly language implementations. The root cause of this bug is that we're not maintaining the invariant `popcount(bitmap) == secondary_supers()->length()`. There are a couple of ways to fix this. We could check `secondary_supers()->length()` before doing any probe. I'm very reluctant to add a memory load to the super-hot path for this edge case, though. It's better to take some pain in the case of an almost-full secondaries array, because those are very rare, and will never occur in most Java programs. So, i've corrected the bitmap at the point the hash table is constructed. ------------- PR Comment: https://git.openjdk.org/jdk/pull/19989#issuecomment-2253019028
