On Fri, 31 Jul 2026 10:17:51 GMT, Marc Chevalier <[email protected]> wrote:
> This PR replaces the Valhalla draft PR > https://github.com/openjdk/valhalla/pull/2642, which was not integrated due > to the code freeze immediately before the Valhalla mainline integration. > > --------- > > It is conceptually very similar to acmp: it has two parts. > > # Static Expansion > > If the operand of `identityHashCode` is known at compile-time, we can > basically inline the implementation of > `ValueObjectMethods.valueObjectHashCode`. There are a few points worth noting. > 1. The seed of the hash is the hash of the mirror class object. That object > is not a value object, but an identity class. We look in the header of the > said class whether it was cached already, which is very likely. Otherwise, we > give up the static expansion: it is not worth replacing a call with a call. > 2. Oops are hell. We don't expand when oops are involved. > 3. The runtime implementation performs some unsafe gets to get all pieces of > a segment in a simple, greedy way. Which means that in the case we have a > value class made of 2 `int`, the likely outcome is that it would get both at > once with a single call to `getLong`. This is fine, we can do that, but we > need to mark the access as mismatch and unsafe (but aligned, since the acmp > maps are smartly done). But in the case we have a class made of a single > `int`, it would be unfortunate to mark the `getInt` access as mismatch since > it prevents some optimizations. So if we are getting exactly a field, we > detect it, and we mark the load as non-mismatch (match?). > > # Fast Path > > This supports only objects with a simple shape: one segment of data, being 1, > 2, 4 or 8-byte long, which is enough to cover many migrated classes (but not > dates for instance). This feature is morally very similar to the acmp fast > path. It is controlled by the diagnostic flag `UseHashcodeFastPath`. Alike > acmp, we need to sabotage the fast path in case we can do a static expansion > later. > > I can only suggest you take a look at `inlineKlass.hpp` on how that works. > > # Benchmarking > > Microbenchmarking is rather unsurprising: > - haven't changed: > - `null` was and is still fast (1-2ns) > - hashcode cached in the header was and is still fast (2ns) > - identity objects without cached hashcode are not too slow, and still > aren't (20-25ns) > - known at compile time (60-70ns): > - value objects with oops > - unknown at compile time (60-70ns): > - value objects with sizes that are not 1, 2, 4 or 8 > - value objects with oops > - got better (60-70ns -> 2-7ns): > - value objects with a type kno... Great work Marc! I did a first pass and added a few comments. Will take another look tomorrow with a fresh brain 😄 src/hotspot/share/opto/callnode.cpp line 1208: > 1206: } > 1207: } > 1208: else if (method()->holder() == phase->C->env()->System_klass() && Suggestion: } else if (method()->holder() == phase->C->env()->System_klass() && src/hotspot/share/opto/inlinetypenode.cpp line 1026: > 1024: > 1025: RegionNode* compute_region = new RegionNode(3); > 1026: igvn.register_new_node_with_optimizer(compute_region); This is never used. src/hotspot/share/opto/inlinetypenode.cpp line 1029: > 1027: > 1028: const Type* arg_type = igvn.type(arg); > 1029: assert(!arg_type->maybe_null(), "must check null beforehand"); Why is this guaranteed? Isn't this only done in the intrinsic slow path but not in the general case? src/hotspot/share/opto/inlinetypenode.cpp line 1038: > 1036: Node* adr = kit->basic_plus_adr(arg, offset); > 1037: ciField* field = vk->get_field_by_offset(offset, false); > 1038: // If the load is by chance not a mismatch, let mark it so. This > way, loading the field can be simplified Suggestion: // If the load is by chance not a mismatch, let's mark it so. This way, loading the field can be simplified src/hotspot/share/opto/inlinetypenode.hpp line 138: > 136: static Node* emit_substitutability_check(GraphKit* kit, Node* lhs, > Node* rhs); > 137: > 138: // Implementation of the substitutability check for acmp This should refer to hashcode src/hotspot/share/opto/library_call.cpp line 5582: > 5580: > 5581: // We only go to the cache case code if we pass a number of guards. > The > 5582: // paths which do not pass are accumulated in the compute_region. The > compute `compute_region` is not defined here. src/hotspot/share/opto/library_call.cpp line 5650: > 5648: // that unmasked_region/unmasked_result merge: > 5649: // 1. the object has no segment, the hash is simply the > hash of the class object > 5650: // 2. the object has one segment of size smaller that 8 > (1, 2, 4) Suggestion: // 2. the object has one segment of size smaller than 8 (1, 2, 4) src/hotspot/share/opto/library_call.cpp line 5677: > 5675: Node* mask = make_load(control(), mask_addr, > TypeLong::LONG, T_LONG, MemNode::unordered); > 5676: Node* obj_extracted = AndL(obj_payload, mask); > 5677: Node* is_long_payload_bol = BoolCmpI(mask, BoolTest::eq, > longcon(-1)); This should be `BoolCompL` because we are comparing longs. src/hotspot/share/opto/library_call.cpp line 5682: > 5680: > 5681: // Case 2. one segment, less than 8-byte long > 5682: Node* result_int = AddI(MulI(intcon(31), result_empty), > ConvL2I(obj_extracted)); I'm not convinced that this is correct for signed `byte` and `short`. Isn't `ConvL2I` omitting the sign bit? test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestHashcodeFastPath.java line 26: > 24: /* > 25: * @test > 26: * @summary Test acmp fast path with value classes Looks like a copy-paste error 🙂 Same below. test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestHashcodeFastPath.java line 471: > 469: int SIZE = 100; > 470: for (int i = 0; i < SIZE; ++i) { > 471: Asserts.assertEQ(h_byte(new Byte((byte)i)), h(new > Byte((byte)i))); Negative values are missing here. test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestHashcodeFastPath.java line 508: > 506: // TODO: complicated structure, static expansion, random values > with/without late removal of fast path > 507: // TODO: all the simple structures, fast path taken if enabled > 508: // TODO: example with oops, no fast path taken, no expansion I think we should file at least a follow-up RFE for these and remove the TODOs from the test (or add the listed cases with this PR). Random values would be good. ------------- Changes requested by thartmann (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/32144#pullrequestreview-4842254211 PR Review Comment: https://git.openjdk.org/jdk/pull/32144#discussion_r3703851013 PR Review Comment: https://git.openjdk.org/jdk/pull/32144#discussion_r3702627414 PR Review Comment: https://git.openjdk.org/jdk/pull/32144#discussion_r3703862654 PR Review Comment: https://git.openjdk.org/jdk/pull/32144#discussion_r3703926434 PR Review Comment: https://git.openjdk.org/jdk/pull/32144#discussion_r3703934635 PR Review Comment: https://git.openjdk.org/jdk/pull/32144#discussion_r3702626179 PR Review Comment: https://git.openjdk.org/jdk/pull/32144#discussion_r3703882391 PR Review Comment: https://git.openjdk.org/jdk/pull/32144#discussion_r3702473445 PR Review Comment: https://git.openjdk.org/jdk/pull/32144#discussion_r3704014235 PR Review Comment: https://git.openjdk.org/jdk/pull/32144#discussion_r3703932146 PR Review Comment: https://git.openjdk.org/jdk/pull/32144#discussion_r3702613912 PR Review Comment: https://git.openjdk.org/jdk/pull/32144#discussion_r3702606860
