On Wed, 11 Feb 2026 21:30:41 GMT, Dan Heidinga <[email protected]> wrote:
>> Hi all, >> >> The main ideas of this patch are to highlight and enforce the invariant we >> enforce when it comes to value objects' identity hash code. >> >> The original JBS issues addresses the following points, which have been >> addressed to various extents: >> >> 1. Adding assertions to the CAS-setting of the hash in the markWord. This is >> vital to enforce the invariant and was added. >> 2. Breaking the loop if the CAS results in a conflict. Putting the identity >> hash in the markWord is an optimization, so one could break out of the loop >> whenever. With the assertion, there's a good confidence that CAS will >> eventually succeed, namely once other threads stop poking at the markWord >> bits. **If there is demand, I can add a fixed upper bound.** >> 3. SSA-ing the hash variable. Done. >> 4. Possibly introducing a markWord::has_hash to improve legibility. I did >> not do this as it would yield multiple `obj->mark()` calls in the fast path >> and the current form is (in my opinion) sufficiently legible. >> >> Testing: tiers 1-3 on Linux (x64, AArch64), macOS (x64, AArch64), Windows >> (x64). > > src/hotspot/share/prims/jvm.cpp line 798: > >> 796: // matter when this is called the same identity hash code is >> expected. >> 797: // 2. Oops: the above still applies, but the oops' identity hash >> code must >> 798: // be used as the polymorphic hashCode may change due to >> mutability. > > This is a good description for generating the identity hashcode but seems > like its in the wrong place... this function doesn't "own" the calculation of > the hash and the various cases - they belong elsewhere. > > The first line `// The generated identity hash is invariantly immutable.` is > the only part of this that applies in this function. > > The reason this comment is odd here is that we don't implement 2 cases here. While this function doesn't own the calculation, my intention with this comment was to demystify the black-box call that is `ValueObjectMethods.valueObjectHashCode` in order to justify why the assertions provided later on are sound. When implementing [JDK-8376171](https://bugs.openjdk.org/browse/JDK-8376171), the VM and Java were using slightly different assumptions, leading to [JDK-8376512](https://bugs.openjdk.org/browse/JDK-8376512). I think that a) this mistake would have been spotted earlier and b) the review would not have taken as long if there was some non-surfacelevel documentation in `IHashCode`. That said, I do agree that there are more applicable/appropriate places to document the behaviour. How would you feel about moving this comment to the JavaDoc of `VOM.valueObjectHashCode` and keeping a shorter summary in this place? ------------- PR Review Comment: https://git.openjdk.org/valhalla/pull/2029#discussion_r2797731124
