Implement fast and slow value object hash paths by AditiS11 · Pull Request #24356 · eclipse-openj9/openj9

AditiS11 · 2026-07-14T11:24:04Z

Use the recursive fast path for value object hash computation.
Use the fast path while the recursion depth is below J9_VALUE_TYPE_HASH_DEPTH_LIMIT.
Switch to the iterative path when the recursion depth limit is exceeded.
Use a DFS-based queue to traverse value object fields in the iterative path.

AditiS11 · 2026-07-14T11:36:13Z

50X Grinder for the test: https://hyc-runtimes-jenkins.swg-devops.com/view/Test_grinder/job/Grinder/61372/testReport/

theresa-m · 2026-07-14T14:57:18Z

+		bool depthExceeded = false;
+		I_32 hashValue = recursiveValueObjectHash(vm, currentThread, objectPointer, clazz, startOffset, 0, &depthExceeded, oomOccurred);
+		if (!depthExceeded || *oomOccurred) {
+			return hashValue;


Shouldn't the method return here regardless of the value of oomOccurred?

theresa-m · 2026-07-14T15:03:11Z

+			if (NULL == entry->currentField->field) {
+				U_32 finalHash = finalizeMurmur3Hash(entry->hashValue, entry->numBytesHashed);
+				if (0 == finalHash) {
+					finalHash = 1;


Can you add back the comment that was here for clarity?
/* Hash code for value types should not be zero. */

theresa-m · 2026-07-14T15:30:37Z

-				default:
-					/* Invalid field signature. Should assert but we are in util. */
-					break;
+				if (*depthExceeded || *oomOccurred) {


Would oomOccurred ever be true here?

No Theresa, oomOccurred would never be true in the recursive path. Will remove this.

hangshao0 · 2026-07-14T15:37:23Z

I suggest adding a debugging -XX option (sth like -XX:VTHashRecDepth=n) to overwrite the default J9_VALUE_TYPE_HASH_DEPTH_LIMIT. This could help us diagnose issues with fastpath vs slowpath implementations.

theresa-m · 2026-07-15T14:00:12Z

+				UDATA depth = 0;
+				char *optname = VMOPT_HASHMAXRECDEPTH_EQUALS;
+				GET_INTEGER_VALUE(argIndex, optname, depth);
+				if (0 != depth) {


I think 0 should be allowed in case we want to test the slow path only.

theresa-m · 2026-07-15T14:08:15Z

+		frame->hashValue = getSalt(vm, (UDATA)frame->objectPointer, true);
+		frame->hashValue = mix(frame->hashValue, (U_32)(UDATA)frame->clazz);
+		frame->numBytesHashed = sizeof(UDATA);
+		frame->currentField = vm->internalVMFunctions->fieldOffsetsStartDo(vm, frame->clazz->romClass, VM_VMHelpers::getSuperclass(frame->clazz),


Please put each of the arguments on a separate line.

theresa-m · 2026-07-15T14:09:22Z

+
+	/**
+	 * Fast-path recursive hash computation for value objects.
+	 * Used if recursive depth is less than J9_VALUE_TYPE_HASH_DEPTH_LIMIT.


J9_VALUE_TYPE_HASH_DEPTH_LIMIT doesn't exist anymore. Also in line 495.

- Use the recursive fast path for value object hash computation. - Use the fast path while the recursion depth is below J9_VALUE_TYPE_HASH_DEPTH_LIMIT. - Switch to the iterative path when the recursion depth limit is exceeded. - Use a DFS-based queue to traverse value object fields in the iterative path. - Add -XX:hashMaxRecDepth debug option to override default recursion depth. Fixes: eclipse-openj9#24087 Signed-off-by: Aditi Srinivas M <Aditi.Srini@ibm.com>

hangshao0 · 2026-07-15T15:38:08Z

+		IDATA parentIndex;
 	};

 	struct ValueTypeHashQueue {


This becomes more of a stack than a queue ?

Yes Hang its a stack. I just kept the naming the same as the original code to minimize the diff.
I will change the naming.

hangshao0 · 2026-07-15T15:42:34Z

 		{
-			if (full) {
+			UDATA newTop = (UDATA)(top + 1);
+			if (newTop >= capacity) {


Why not check if (newTop == capacity) only ?

I just added >= as a defensive check. Will change it to newTop == capacity

hangshao0 · 2026-07-15T15:46:55Z

+								UDATA hashSlotOffset = fieldClazz->backfillOffset;
+								storedHash = objectAccessBarrier.inlineMixedObjectReadI32(currentThread, fieldObject, hashSlotOffset);
 							}
+							if (0 != storedHash) {


I think for non-VT field, its hash can be 0. So if the storedHash is 0 in this case, it can still be used.
The same comment apply to the slowpath.

Based on the analysis of one of the benchmark logs, we didn't observe any cases where identity types had a hash value of 0. Hence I assumed this scenario wouldn't occur.

dmitripivkine · 2026-07-15T16:58:28Z

@hangshao0 How we guarantee that object pointers stored during calculation (slow path particularly) are not becoming stale? Do we assume hash code calculation executes under VM Access always to prevent GC?
If so,

Should assertion VM Access is taken be added?
Should here be a concern for delaying GC for too long potentially?

hangshao0 · 2026-07-15T18:40:16Z

The code here in this PR assumes the object pointers are non-stale.

Should assertion VM Access is taken be added?

We should have an assertion for this. However, there were review comments on adding assertions in ObjectHash,hpp in Aditi's previous hashcode implementation PR. but it seems assertions cannot be added into this file (don't recalled the details back then). If that is the case, I think assertion should be added to the callers in other files for VM access (it should be taken if not already taken).

Should here be a concern for delaying GC for too long potentially?

I expect VT objects are mostly leaves of the data tree that does not delay GC too long for the hash calculation.

dmitripivkine · 2026-07-15T19:31:10Z

yes, assertion can be tricky here

AditiS11 force-pushed the value2 branch 3 times, most recently from 8fac93c to a8c82ae Compare July 14, 2026 11:32

theresa-m added comp:vm project:valhalla Used to track Project Valhalla related work labels Jul 14, 2026

hangshao0 requested review from dmitripivkine, hangshao0 and theresa-m July 14, 2026 15:30

theresa-m suggested changes Jul 14, 2026

View reviewed changes

AditiS11 force-pushed the value2 branch from a8c82ae to 31fc9de Compare July 15, 2026 08:44

theresa-m suggested changes Jul 15, 2026

View reviewed changes

AditiS11 force-pushed the value2 branch from 31fc9de to 7d8c1e1 Compare July 15, 2026 14:37

hangshao0 reviewed Jul 15, 2026

View reviewed changes

Uh oh!

Conversation

AditiS11 commented Jul 14, 2026

Uh oh!

AditiS11 commented Jul 14, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hangshao0 commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitripivkine commented Jul 15, 2026

Uh oh!

hangshao0 commented Jul 15, 2026

Uh oh!

dmitripivkine commented Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hangshao0 commented Jul 14, 2026 •

edited

Loading