On Wed, 26 Aug 2026 15:40:49 GMT, Axel Boldt-Christmas <[email protected]> wrote:
>> Historically we have had displaced markWord's because locking (stack locks, >> and inflated monitors). >> >> After the `UseObjectMonitorTable` removal there are no more need to protect >> against displaced markWords. >> >> I suggest we simplify this logic and clean up the interface w.r.t. >> `identity_hash`. >> >> Changes the `fast_no_hash_check` and `has_no_hash` into `has_identity_hash` >> and `has_hash` checks. >> As before it is invalid to read the hash on marked objects, added asserts to >> ensure this. >> Rewrote `oopDesc::slow_identity_hash` to not regenerate a new hash every >> time it transiently fails to install the hash in the markWord due to some >> other header change. >> >> There are a few places which right now calls `identity_hash` from develop >> logging / introspection printing `InstanceStackChunkKlass::print_chunk` and >> `Continuation::print`. It is a bit unfortunate that we have these >> side-effects in debug VMs. But think we should handle these in a future RFE. >> >> Testing (in progress): >> * Tier 1-3 Oracle supported platforms >> * GHA >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Axel Boldt-Christmas has updated the pull request incrementally with one > additional commit since the last revision: > > Update Inline type identity hash comment I've looked over the PR and think this looks good. I've added a number of nits and comments about things that could be fixed in this PR, or handled separately in a follow-up PR, or maybe even ignored :) src/hotspot/share/cds/aotMappedHeapWriter.cpp line 745: > 743: // We need to retain the identity_hash, because it may have been used > by some hashtables > 744: // in the shared heap. > 745: if (src_obj->has_identity_hash() && > (!(Arguments::is_valhalla_enabled() && src_obj->mark().is_inline_type()))) { I think this code reads a bit funny. I would prefer if we didn't call `has_identity_has` on identityless objects (value objects). I wonder if this can be turned around to be: Suggestion: if (!src_obj->is_inline_type() && src_obj->has_identity_hash()) { or even if we could have an identity-check and: Suggestion: if (src_obj->has_identity() && !src_obj->has_identity_hash()) { src/hotspot/share/cds/aotMappedHeapWriter.cpp line 750: > 748: fake_oop->set_mark(fake_oop->mark().copy_set_hash(src_hash)); > 749: } else if (Arguments::is_valhalla_enabled()) { > 750: > fake_oop->set_mark(src_klass->prototype_header().copy_set_hash(src_hash)); BTW, what is this code all about? Here we know that Valhalla is enabled and that the object is not a value object, so why do we need to use the `prototype_header()` for this? Maybe something to look into outside this PR. src/hotspot/share/oops/oop.cpp line 121: > 119: precond(!mark.has_hash()); > 120: > 121: // VM should be calling bootstrap method. I have no idea what this old comment is trying to convey. I propose removing it with this PR. Suggestion: src/hotspot/share/oops/oop.cpp line 122: > 120: > 121: // VM should be calling bootstrap method. > 122: assert(!klass()->is_inline_klass(), "slow_identity_hash should not be > called for inline classes"); Pre-existing: The comment is a little bit off. Because it is the object and not the class that we are calling slow_identity_hash on. Suggestion: assert(!is_inline(), "slow_identity_hash should not be called for value objects"); src/hotspot/share/oops/oop.cpp line 125: > 123: > 124: // Calculate the new hash > 125: const intptr_t new_hash = ObjectSynchronizer::get_next_hash(current, > this); // get a new hash Drop the old comment now that you have a new comment above: Suggestion: const intptr_t new_hash = ObjectSynchronizer::get_next_hash(current, this); src/hotspot/share/oops/oop.cpp line 125: > 123: > 124: // Calculate the new hash > 125: const intptr_t new_hash = ObjectSynchronizer::get_next_hash(current, > this); // get a new hash If you moved the `FastHashCode` implementation to oop.cpp, I think it would make a lot of sense to also move the `get_next_hash` implementation here. src/hotspot/share/oops/oop.cpp line 132: > 130: > 131: // Try to install the hash > 132: mark = cas_set_mark(new_mark, old_mark, memory_order_relaxed); Pre-existing: We tend to stay away from rewriting the value for input arguments, to make it easier to read the code. It would be nice if this could be rewritten to follow that unwritten guideline. We can discuss that as a potential follow-up. src/hotspot/share/oops/oop.inline.hpp line 407: > 405: } > 406: > 407: intptr_t oopDesc::identity_hash(Thread* current) { Pre-existing: It is only the slow-path that makes sure that this isn't called for value objects. Should we check it in the fast-path as well? src/hotspot/share/prims/jvm.cpp line 794: > 792: oop obj = JNIHandles::resolve_non_null(handle); > 793: if (Arguments::is_valhalla_enabled() && > obj->klass()->is_inline_klass()) { > 794: const intptr_t obj_identity_hash = obj->mark().hash(); Just a thought for future discussion: maybe we shouldn't use the word "identity" for the variables and comments when we're dealing with an identity-less value object. Maybe just call it hash? ------------- Marked as reviewed by stefank (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/32535#pullrequestreview-5033614471 PR Review Comment: https://git.openjdk.org/jdk/pull/32535#discussion_r3865596668 PR Review Comment: https://git.openjdk.org/jdk/pull/32535#discussion_r3865612446 PR Review Comment: https://git.openjdk.org/jdk/pull/32535#discussion_r3865434706 PR Review Comment: https://git.openjdk.org/jdk/pull/32535#discussion_r3865457045 PR Review Comment: https://git.openjdk.org/jdk/pull/32535#discussion_r3865444277 PR Review Comment: https://git.openjdk.org/jdk/pull/32535#discussion_r3865473333 PR Review Comment: https://git.openjdk.org/jdk/pull/32535#discussion_r3865504780 PR Review Comment: https://git.openjdk.org/jdk/pull/32535#discussion_r3865538663 PR Review Comment: https://git.openjdk.org/jdk/pull/32535#discussion_r3865647146
