Implement native method to calculate hashcode for valuetypes by AditiS11 · Pull Request #23502 · eclipse-openj9/openj9

AditiS11 · 2026-03-12T16:26:58Z

-Add native method to calculate hashcode for valuetypes.
-Modify GC path to return/store 0 for value types,
to eliminate hashcode calculation during object copying.
-Ensure hashcode calculation is only triggered by the VM.
-Enable ValueTypeTests:testValueClassHashCode

Fixes: #15768

This change follows the approach described in 15768(comment)

AditiS11 · 2026-03-12T16:28:48Z

With these changes the following OpenJDK tests are passing:

valhalla/valuetypes/Reflection.java
valhalla/valuetypes/StreamTest.java
valhalla/valuetypes/RecursiveValueClass.java

theresa-m

First pass.

theresa-m · 2026-03-12T16:32:14Z

-	/* https://github.com/eclipse-openj9/openj9/issues/15768 */
-	@Test(priority=1, enabled = false)
+	@Test(priority=1)
 	static public void testValueClassHashCode() throws Throwable {


Are there any tests for value classes with identity fields (here or in OpenJDK)?

No, I don't think so. I will add another test similar to 15768. Will open another PR for that.
Verified 15768 with the current changes locally, and the behaviour matches the ri.

theresa-m · 2026-03-12T16:34:12Z

@@ -70,7 +70,6 @@ final class J9VMInternals {

 	/*[IF INLINE-TYPES]*/
 	static boolean positiveOnlyHashcodes = positiveOnlyHashcodes();


This field and method positiveOnlyHashcodes() can also be removed.

theresa-m · 2026-03-13T14:59:15Z

 private:
 protected:
 public:
+	struct ValueTypeHashQueueEntry {


This struct and the new helper methods can be under private in this file.

I've encountered compilers that don't handle private types properly in all situations.

theresa-m · 2026-03-13T15:00:54Z

+	 * Use inlineObjectHashCode for reference fields.
+	 * Use a queue to iterate through the fields of a valuetype.
+	 *
+	 * @param vm		a java VM


currentThread is missing in this comment.

nit: spaces over tabs to separate the field descriptions.

theresa-m · 2026-03-13T15:23:33Z

+	{
+		I_32 hashValue = 0;
+		J9Class *clazz = J9OBJECT_CLAZZ(currentThread, ((uintptr_t)objectPointer));
+		 if ((NULL != clazz) && J9_IS_J9CLASS_VALUETYPE(clazz)) {


In what scenario could clazz be null?

These were just some defensive checks which I added while debugging some crashes earlier. Will remove it.

theresa-m · 2026-03-13T15:27:46Z

+		UDATA queueSize = initialQueueSize;
+		ValueTypeHashQueueEntry *queue = (ValueTypeHashQueueEntry *)j9mem_allocate_memory(sizeof(ValueTypeHashQueueEntry) * queueSize, OMRMEM_CATEGORY_VM);
+		if (NULL == queue) {
+			return 0;


Failure to allocate memory should result in an out of memory exception.

theresa-m · 2026-03-13T15:32:31Z

+			ValueTypeHashQueueEntry entry = queue[head++];
+			J9ROMFieldOffsetWalkState state;
+			J9ROMFieldOffsetWalkResult *result = vm->internalVMFunctions->fieldOffsetsStartDo(vm, entry.clazz->romClass, VM_VMHelpers::getSuperclass(entry.clazz), &state, J9VM_FIELD_OFFSET_WALK_INCLUDE_INSTANCE, entry.clazz->flattenedClassCache);
+			while ((NULL != result) && (NULL != result->field)) {


why would result->field be null?

fieldOffsetsNextDo returns a pointer to the result structure, but sets result->field to NULL to indicate the end of field traversal.

I see. result->field should be sufficient then, result will not be null.

theresa-m · 2026-03-13T15:40:26Z

+#endif /* defined(J9VM_OPT_VALHALLA_FLATTENABLE_VALUE_TYPES) */
+						{
+							j9object_t fieldObject =  objectAccessBarrier.inlineMixedObjectReadObject(currentThread, entry.objectPointer, fieldOffset);
+							if (NULL != fieldObject) {


Why would this be null?

fieldObject could be null if the reference fields are uninitialized.

theresa-m · 2026-03-13T15:43:31Z

+				result = vm->internalVMFunctions->fieldOffsetsNextDo(&state);
+			}
+		}
+		const U_32 MUL1 = 0x85ebca6b;


Can you create a helper method to avoid this duplicated code between here and inlineConvertValueToHash ?

theresa-m · 2026-03-13T15:47:37Z

+					case 'L': {
+						U_32 datum = 0;
+#if defined(J9VM_OPT_VALHALLA_FLATTENABLE_VALUE_TYPES)
+						if (vm->internalVMFunctions->isFlattenableFieldFlattened(entry.clazz, result->field)) {


I think it would be better to handle the L case as either value type fields or instance fields, not flattened fields and non-flattened fields. This will avoid unnecessary searches in the flattened class cache here and hopefully avoid some duplicated code.

I tried restructuring the L case, but I got a segfault when reading the field as an object reference. Reading a flattened value field using inlineMixedObjectReadObject() results in an invalid object pointer.
I think we need to read the reference and use J9OBJECT_CLAZZ(currentThread, fieldObject) to determine whether it is a value type or a regular instance.

Oh right. It's fine as is then.

You could check J9ROMFIELD_IS_NULL_RESTRICTED like in isSubstitutable.

theresa-m

What is the behavior of the ri if hashCode is called prior to all strict instance fields being set in ? The easiest way to test this out is to use the @NullRestricted annotation which will mark the instance field as strict.

theresa-m · 2026-03-18T17:56:28Z

 		return EXECUTE_BYTECODE;
 	}

-	/* java.lang.J9VMInternals: positiveOnlyHashcodes() */


Remove extra line 3161.

theresa-m · 2026-03-18T17:59:43Z

 		/* Caller has null-checked obj already */
 		_pc += 3;
-		*(I_32*)_sp = VM_ObjectHash::inlineObjectHashCode(_vm, obj);
+		I_32 hashValue = VM_ObjectHash::inlineObjectHashCode(_vm, obj);


Since oom isn't a factor for non-valhalla builds I suggest keeping these additions under a build flag. Also in ObjectHash.hpp.

theresa-m · 2026-03-18T18:05:56Z

 * Data members
 */
 private:
+	struct ValueTypeHashQueueEntry {


This struct and most of the new helper methods should be under value type compiler flags.

theresa-m · 2026-03-18T18:17:59Z

+					case 'S': { /* short */
+						U_32 datum = objectAccessBarrier.inlineMixedObjectReadU32(currentThread, entry.objectPointer, fieldOffset);
+						hashValue = mix(hashValue, datum);
+						numBytesHashed+=4;


numBytesHashed += 4;

theresa-m · 2026-03-18T18:18:51Z

+							queue[tail].startOffset = fieldOffset;
+							tail++;
+							break;
+						}


Should there be an else here?

theresa-m · 2026-03-18T18:20:24Z

+						break;
+					}
+					default:
+						  break;


This should result in an error.

I initially thought of using Assert_VM_unreachable(), but I think it can't be used here.
Could you please suggest on what error to throw here.

Use Assert_VM_unreachable() (or similar) seems reasonable; reaching line 324 means the signature is invalid - which should have been detected long before we get here.

I think you can add a comment like https://github.com/eclipse-openj9/openj9/blob/master/runtime/oti/ObjectHash.hpp#L121 instead. edit: didn't see Keith's answer before posting this.

theresa-m · 2026-03-18T18:20:53Z

+	};
 protected:
 public:
-


This line does not need to be removed.

keithc-ca · 2026-03-19T17:36:15Z

 	/**
 	 * Determine the basic hash code for the specified object. This may modify the object. For example, it may
 	 * set the HAS_BEEN_HASHED bit in the object's header. Object must not be NULL.
+	 * Does not calculate hashcode for valuetypes. Returns 0.


Please spell "hash code" consistently.
Why is zero the right answer for value types?

Followed this as per the discussion in #15768 (comment).
Valuetype hash calculation is not performed during GC as objects may still be moving.

That's not how I read that comment. Zero might signal an object in transit, but when the hash code is computed later it need not be zero. Where is the code that computes that (real) hash code, if not this?

keithc-ca · 2026-03-19T17:37:19Z

+#if defined(J9VM_OPT_VALHALLA_VALUE_TYPES)
+		if (isValueType) {
+			return 0;
+		}


Why is this only for those two GC modes?

This is for all the GC modes. Sorry about this, I have changed it.

keithc-ca · 2026-03-19T17:39:41Z

+#if defined(J9VM_OPT_VALHALLA_VALUE_TYPES)
+		J9Class *objectClass = J9GC_J9OBJECT_CLAZZ(objectPtr, this);
+		if (J9_IS_J9CLASS_VALUETYPE(objectClass)) {
+			*hashCodePointer = 0;
+		} else
+#endif /* defined(J9VM_OPT_VALHALLA_VALUE_TYPES) */


Again, shouldn't this be independent of the GC policy?

The entire function body is under the if defined (OMR_GC_MODRON_COMPACTION) || defined (J9VM_GC_GENERATIONAL) flag. Hence I added my change following the same structure.

Just to clarify: this ifdef is an ugly check of condition "object can be moved by GC", so there are two cases obviously:

objects can be moved

objects always stay at the same place never change location.

keithc-ca · 2026-03-19T17:41:22Z

 private:
 protected:
 public:
+	struct ValueTypeHashQueueEntry {


I've encountered compilers that don't handle private types properly in all situations.

keithc-ca · 2026-03-19T17:42:45Z

+	 * @param[in] tail pointer to one index past the last active element
+	 * @return pointer to resized queue, or NULL if allocation fails.
+	 */
+	static VMINLINE ValueTypeHashQueueEntry*


Space before *, please.

keithc-ca · 2026-03-19T17:47:19Z

+					case 'Z': /* boolean */
+					case 'B': /* byte */
+					case 'C': /* char */
+					case 'I': /* int */
+					case 'F': /* float */
+					case 'S': { /* short */


Our coding standard says case labels should indented the same as switch.
Suggest 'S' and the comment should be closer:

case 'S': /* short */ {

keithc-ca · 2026-03-19T17:49:46Z

+	{
+		PORT_ACCESS_FROM_JAVAVM(vm);
+		MM_ObjectAccessBarrierAPI objectAccessBarrier(currentThread);
+		I_32 hashValue = getSalt(vm, (UDATA) objectPointer);


Please format casts consistently: others don't have the space.

keithc-ca · 2026-03-19T17:52:04Z

+							queue[tail].objectPointer = entry.objectPointer;
+							queue[tail].clazz = flatClazz;
+							queue[tail].startOffset = fieldOffset;
+							tail++;


Please use +=; here and line 315.

keithc-ca · 2026-03-19T17:52:27Z

+				result = vm->internalVMFunctions->fieldOffsetsNextDo(&state);
+			}
+			if ((NULL != queue) && (head < tail)) {
+				entry = queue[head++];


The increment should be separate:

entry = queue[head]; head += 1;

keithc-ca · 2026-03-19T17:52:50Z

+
+		hashValue = finalizeMurmur3Hash(hashValue, numBytesHashed);
+done:
+		if (NULL!= queue) {


Missing space before !=.

AditiS11 · 2026-03-20T18:22:18Z

What is the behavior of the ri if hashCode is called prior to all strict instance fields being set in ? The easiest way to test this out is to use the @NullRestricted annotation which will mark the instance field as strict.

It throws a compiler error: reference to hashCode() may only appear after an explicit constructor invocation

theresa-m

I suggest summarizing how hashcodes are updated for value types or link #15768 (comment) in the description so it is easy to find.

theresa-m · 2026-03-23T14:08:22Z

 		/* Caller has null-checked obj already */
 		_pc += 3;
-		*(I_32*)_sp = VM_ObjectHash::inlineObjectHashCode(_vm, obj);
+		I_32 hashValue = VM_ObjectHash::inlineObjectHashCode(_vm, obj);


Fields should be declared at the top of the method.

This is C++ code; hashValue is a local which should be declared like this as it gets its first (better - only) value.

theresa-m · 2026-03-23T14:12:09Z

 	 *
 	 * @param vm			a java VM
 	 * @param objectPointer 	a valid object reference
 	 */


This comment should be updated to show that the oom could be returned.

keithc-ca · 2026-03-23T21:24:57Z

+		} else
+#endif /* defined(J9VM_OPT_VALHALLA_VALUE_TYPES) */
+		{
+			 *hashCodePointer = convertValueToHash(vm, (uintptr_t)objectPtr);


Please fix the indentation here (only tabs before *).

keithc-ca · 2026-03-23T21:36:36Z

+
+		while (true) {
+			J9ROMFieldOffsetWalkState state;
+			J9ROMFieldOffsetWalkResult *result = vm->internalVMFunctions->fieldOffsetsStartDo(vm, entry.clazz->romClass, VM_VMHelpers::getSuperclass(entry.clazz), &state, J9VM_FIELD_OFFSET_WALK_INCLUDE_INSTANCE, entry.clazz->flattenedClassCache);


This line is much too long:

J9ROMFieldOffsetWalkResult *result = vm->internalVMFunctions->fieldOffsetsStartDo( vm, entry.clazz->romClass, VM_VMHelpers::getSuperclass(entry.clazz), &state, J9VM_FIELD_OFFSET_WALK_INCLUDE_INSTANCE, entry.clazz->flattenedClassCache);

keithc-ca · 2026-03-23T21:37:13Z

+								 if (NULL == queue) {
+									 hashValue = -1;
+									 goto done;
+								 }


Please correct the indentation here.

keithc-ca · 2026-03-23T21:37:42Z

+					break;
+				}
+				default:
+					  /* Invalid field signature. Should assert but we are in util. */


Please correct the indentation here.

keithc-ca · 2026-03-23T21:38:13Z

+		}
+		return hashValue;
+	}
+#endif /* if defined(J9VM_OPT_VALHALLA_VALUE_TYPES) */


Please remove "if" (only the condition should appear here).

keithc-ca · 2026-03-23T21:41:01Z

+	 * @param objectPointer a valid valuetype object
+	 * @param clazz class of the valuetye object
+	 * @param startOffset offset to start reading fields from
+	 * @return the calculated hash code or -1 if an OOM occurred


-1 is a perfectly reasonable hash value (you might expect that for an Integer with the value -1: some other mechanism is needed to distinguish OOM.

In the current design, the hash slot stores 0 for value types until a hash is computed by the VM. Since 0 already represents the “hash not computed” case for value types, would it make sense to also return 0 from this function if an OOM occurs?

I think the alternative would be to add an flag (like oomOccurred parameter) to the function signature to check for this case, but that would require updating all the callers of this function.

I only see one call to this function.

The OOM condition is currently handled in BytecodeInterpreter.hpp. When inlineObjectHashCode() returns -1, it is treated as an indication that an OOM occurred and the appropriate exception is set.

If we introduce an additional parameter such as oomOccurred to propagate this information, I think the function signatures would need to change, all calls of these functions should be updated as well.

What's the objection? We need function signatures that handle all reasonable scenarios.

dmitripivkine · 2026-03-23T22:48:29Z

+		if (J9_IS_J9CLASS_VALUETYPE(objectClass)) {
+			return 0;
+		}
+#endif /* defined(J9VM_OPT_VALHALLA_VALUE_TYPES) */


I think this is not good enough in the case you want to store hash code in VT object eventually.
If you look to the logic below, you can see:

If object has been moved (has hash slot inflated already and "has been moved" flag set) read hash code from the object directly, no calculation. For VT object it means read code from the object, check for 0 (has not been stored yet, this is first hash code request after move), calculate code and store it in the object. If hash is not 0 (has been stored already) use this code.

In the "else" case object has not been moved yet, so hash code should be calculated any way, but "has been hashed" flag must be set (atomically) in order to inflate hash slot for VT object when it is going to be moved. We are not going to be able to calculate hash code for VT object at that time and keep it zero, but this code can be updated next time moved VT object hash is requested.
Current code does not set has been hashed" flag, so slot is not going to be inflated during move.

Thank you for the explanation, I have updated the code to set the HAS_BEEN_HASHED flag for value types before returning 0.

Assuming new code does what it is intended, it should be better structured. Current implementation looks ugly. My first thought is can VT check be integrated in convertValueToHash() and computeObjectAddressToHash() instead to be explicit at the caller (ex. adding extra parameter to control desired behaviour). If, by some reason it is inconvenient, we should create local wrapper functions in this class to hide the check.

Also please point me where inflated hash slot for VT object is updated by VM? I am not objecting this design point but this function is a good candidate to do this.

Also please point me where inflated hash slot for VT object is updated by VM? I am not objecting this design point but this function is a good candidate to do this.

https://github.com/eclipse-openj9/openj9/pull/23502/changes#diff-9aa50703bc7320e2410d7093b1f619ded056a3aeed5f75e2477603d7a5772712R543-R550

In inlineObjectHashCode() the VM first reads the value from the backfill offset. If the value is 0, it indicates that the hash has not yet been computed, so inlineConvertValueObjectToHash() is called to calculate the hash for valuetypes. The computed hash is then written back to the backfill slot.

When VM_VMHelpers::mustCallWriteAccessBarrier(vm) is true, the VM delegates to j9gc_objaccess_getObjectHashCode(). In the GC path, the function currently returns 0 for value types when the hash has not yet been calculated.

My understanding is that in this case the VM should detect the 0 value and then trigger the value type hash calculation, which is missing in the current implementation. Please correct me if I am misunderstanding this flow.

Also, since you mentioned that this function could be a good place to update the inflated hash slot for value types, I am not entirely sure how that would be done here. My understanding is that the value type hash calculation currently happens in the VM (inlineConvertValueObjectToHash()), while getObjectHashCode() is invoked through the GC access barrier path.

dmitripivkine · 2026-03-24T13:55:52Z

+#if defined(J9VM_OPT_VALHALLA_VALUE_TYPES)
+		J9Class *objectClass = J9GC_J9OBJECT_CLAZZ(objectPtr, this);
+		if (J9_IS_J9CLASS_VALUETYPE(objectClass)) {
+			*hashCodePointer = 0;


This point needs extra clarification regarding VT objects, but general idea is:

There is "Pre-hash" optimization: customer can specify list of classes instances of which are going to be allocated with inflated and filled hash slot from the beginning.

This function is used to provide has slot initialization for this optimization and called from object allocation path.

I guess VT object hashing support might want to use this feature. So, I am not sure returning 0 is a right answer.

Yes, this hash slot can be updated later on using "update after VT object move" general logic, but I think real hash code can be calculated and stored right here.

dmitripivkine · 2026-03-26T15:10:28Z

Sorry, I probably missed something, I was under impression that during VT hash code calculation if visited sub-object has hash code stored in the slot already, this code can be used instead of walking this object again. Is it right? If so, where it is implemented in the code? Just want to double check.

AditiS11 · 2026-03-26T17:52:14Z

Sorry, I probably missed something, I was under impression that during VT hash code calculation if visited sub-object has hash code stored in the slot already, this code can be used instead of walking this object again. Is it right? If so, where it is implemented in the code? Just want to double check.

https://github.com/eclipse-openj9/openj9/pull/23502/changes#diff-9aa50703bc7320e2410d7093b1f619ded056a3aeed5f75e2477603d7a5772712R269-R329
The current implementation relies on inlineObjectHashCode() for reference fields. This helper checks whether the object already has a hash stored in its hash slot and returns it if present; otherwise it computes and stores a new one.

For value types, the hash slot is not checked, and the current implementation walks through all the fields to compute the hash.

-Add native method to calculate hashcode for valuetypes. -Modify GC path to return/store 0 for value types, to eliminate hashcode calculation during object copying. -Ensure hashcode calculation is only triggered by the VM -Enable ValueTypeTests:testValueClassHashCode Fixes: eclipse-openj9#15768 Signed-off-by: Aditi Srinivas M <Aditi.Srini@ibm.com>

-Add function to skip valuetype hash calculation in getObjectHashCode() -Add the oomOccurred parameter to handle OOM.

-Modify getObjectHashCode() to udate the hash slot for valuetypes. -Update initializeHashSlot() to calculate the actual hash for valuetypes.

dmitripivkine · 2026-04-10T19:43:56Z

@AditiS11 @theresa-m
My concern is complicate logic structure hard to read and follow.
I am suggesting even before apply this PR logic completely eliminate computeObjectAddressToHash chain (including inlinedCompute... and use convertValueToHash() chain of functions instead. And do it in separate PR before add VT changes.
If we look close to computeObjectAddressToHash chain, it ends by calling inlineConvertValueToHash inside inlineComputeObjectAddressToHash. I think all this can be replaced by calling convertValueToHash() instead.
With this changed, logic of this PR became much simpler and cleaner.
If you like I can do this prerequisite change myself, please let me know if you prefer this.

dmitripivkine · 2026-04-13T12:40:53Z

Sorry, my previous suggestion might be not so good as I thought. It is cleaning code and simplification indeed, but I have missed the fact that VT hash calculation requires object to work with. It means using "convert" path where object address is casted to the value upfront is not optimum direction to move.

-Handle OOM when objectHashCode() is called -Add convertValueToHashForObjectWithForwardedHeader which accepts class as a parameter

dmitripivkine · 2026-04-13T15:20:23Z

 	}

+	MMINLINE I_32
+	convertValueToHashForObject(J9JavaVM *vm, UDATA value, j9object_t object)


Sorry, I was not clear. There is no need to have value as parameter, use cast (uintptr_t)object instead. So, signature can be (J9JavaVM *vm, j9object_t object).

dmitripivkine · 2026-04-13T15:25:58Z

+	}
+
+	MMINLINE I_32
+	convertValueToHashForObjectWithForwardedHeader(J9JavaVM *vm, UDATA value, J9Class *clazz)


You can use the same convertValueToHashForObject name for function. Compiler can recognize different signature and take it.
If you want to mention Forwarded Header in the name, it can be convertValueToHashForForwardedHeader or convertValueToHashForObjectFromForwardedHeader

AditiS11 · 2026-04-14T11:57:29Z

I just had a thought about the current implementation.
Does it make sense to have a separate hash code path for valuetypes like the current implementation (valueHashCode) instead of modifying inlineObjectHashCode to handle both identity and value types.
I am not sure what implication it will have in the places where objectHashCode(which will handle only identity types) is called.
If we have a separate path for value bjects, we will not have to modify the definition of inlineObjectHashCode to handle OOM.
I initially stared off from a Draft PR and hence went ahead with the approach of modifying inlineObjectHashCode to handle both identity and value types.

dmitripivkine · 2026-04-14T14:28:00Z

 		return result;
 	}

+	MMINLINE I_32


This file uses OMR recommended (native) declaration types. int32_t should be used here and below instead of I_32.

theresa-m · 2026-04-08T21:02:59Z

+	static VMINLINE I_32
+	inlineConvertValueObjectToHash(J9JavaVM *vm, J9VMThread *currentThread, j9object_t objectPointer, J9Class *clazz, bool *oomOccurred)
+	{
+		return convertValueObjectAtOffsetToHash(vm, currentThread, objectPointer, clazz, J9VMTHREAD_OBJECT_HEADER_SIZE(currentThread), oomOccurred);


Is there a reason to have convertValueObjectAtOffsetToHash separated into a different function?

Tried following the existing pattern.
For identity types inlineConvertValueToHash has the main logic and inlineComputeObjectAddressToHash is a wrapper around it.

I see. I think this makes some sense inlineComputeObjectAddressToHash since its used as an api for the gc but is not necessary for this function.

theresa-m · 2026-04-14T19:13:34Z

+	 * @return the calculated hash code or 0 if oom occurred
+	 */
+	static VMINLINE I_32
+	convertObjectToHash(J9JavaVM *vm, J9VMThread *currentThread, j9object_t objectPointer, bool *oomOccurred)


I think it would be more consistent to call this inlineConvertObjectToHash

theresa-m · 2026-04-14T19:16:09Z


+#if defined(J9VM_OPT_VALHALLA_VALUE_TYPES)
+I_32
+convertValueTypeToHash(J9JavaVM *vm, J9VMThread *currentThread, j9object_t objectPointer, J9Class *clazz, jboolean *oomOccurred)


Why not just call convertObjectToHash ?

hangshao0 · 2026-04-14T20:38:40Z

I just had a thought about the current implementation.
Does it make sense to have a separate hash code path for valuetypes like the current implementation (valueHashCode) instead of modifying inlineObjectHashCode to handle both identity and value types.
I am not sure what implication it will have in the places where objectHashCode(which will handle only identity types) is called.
If we have a separate path for value bjects, we will not have to modify the definition of inlineObjectHashCode to handle OOM.
I initially stared off from a Draft PR and hence went ahead with the approach of modifying inlineObjectHashCode to handle both identity and value types.

If you mean sth like the following, it makes sense to me (only to deal with OOM for value type).

I_32
objectHashCode(J9JavaVM *vm, j9object_t objectPointer, BOOLEAN* OOMOccurred)
{
    if (objectPointer is value type) {
          /* Value type version to calculate hashcode which could be OOM. */
    } else 
         return VM_ObjectHash::inlineObjectHashCode(vm, objectPointer);
    }
}

theresa-m · 2026-04-20T15:51:13Z

 				info->lockedMonitors.arr_safe[i].clazz =
 					vmfns->j9jni_createLocalRef(env, J9VM_J9CLASS_TO_HEAPCLASS(J9OBJECT_CLAZZ((J9VMThread *)env, object)));
+#if defined(J9VM_OPT_VALHALLA_VALUE_TYPES)
+				jboolean oomOccurred = JNI_FALSE;


Since this is c code new variables should be declared directly after the for loop at 2077.

theresa-m · 2026-04-20T15:53:11Z

+					info->lockedMonitors.arr_safe[i].identityHashCode = hashValue;
+				} else {
+					exc = J9VMCONSTANTPOOL_JAVALANGOUTOFMEMORYERROR;
+					break;


Instead of breaking here this should be reorganized to fall through to 2100, releasing allocated memory.

theresa-m · 2026-04-20T15:54:13Z


 		obj = *((j9object_t *)object);
+#if defined(J9VM_OPT_VALHALLA_VALUE_TYPES)
+		jboolean oomOccurred = JNI_FALSE;


These should be initialized after line 88.

theresa-m · 2026-04-20T15:59:16Z

 Fast_java_lang_J9VMInternals_identityHashCode(J9VMThread *currentThread, j9object_t objectPointer)
 {
-	return VM_ObjectHash::inlineObjectHashCode(currentThread->javaVM, objectPointer);
+	I_32 hashValue = 0;


nit: hashValue is only needed for the value type case, the non value type case can directly return the result.

theresa-m · 2026-04-20T16:55:45Z

+		if (JNI_FALSE == oomOccurred) {
+			 hash = (UDATA)(U_32)hashValue;
+		} else {
+			hash = 0;


Instead of returning 0 here which may make a class hard to find in the hash table I would suggest as assertion. Assert_VM_false(oomOccurred).

theresa-m · 2026-04-20T17:04:11Z

+#if defined(J9VM_OPT_VALHALLA_VALUE_TYPES)
+	bool oomOccurredBool = false;
+	I_32 result = VM_ObjectHash::inlineObjectHashCode(vm, objectPointer, &oomOccurredBool);
+	if (NULL != oomOccurred) {


Are there any scenarios where oomOccured is expected to be null? If not this can be removed.

theresa-m · 2026-04-20T17:05:24Z

+#else /* defined(J9VM_OPT_VALHALLA_VALUE_TYPES) */
+	I_32 result = VM_ObjectHash::inlineObjectHashCode(vm, objectPointer);
+#endif /* defined(J9VM_OPT_VALHALLA_VALUE_TYPES) */
+	return result;


Does this compile? There are two return statements for the J9VM_OPT_VALHALLA_VALUE_TYPES case.

theresa-m · 2026-04-20T17:10:34Z

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


If the stored hash is 0 shouldn't this object be added to the queue and is hash code calculated? I assume this would be a value type.

theresa-m · 2026-04-20T17:11:26Z

+								break;
+							}
+						} else if (J9_IS_J9CLASS_VALUETYPE(fieldClazz)) {
+							if (NULL == queue) {


I would recommend splitting out the queue creation code into a separate method since this one is already quite long and this code is repeated.

theresa-m · 2026-04-20T17:12:21Z

+					U_32 datum = 0;
+#if defined(J9VM_OPT_VALHALLA_FLATTENABLE_VALUE_TYPES)
+					if (J9ROMFIELD_IS_NULL_RESTRICTED(result->field)) {
+						if (vm->internalVMFunctions->isFlattenableFieldFlattened(entry.clazz, result->field)) {


Looks like you are missing the case for a null-restricted field that is not flattened.

- Overload the function convertValueToHashForObjecti() to take the J9Class structure from forwarded header -Remove inlineConvertValueObjectToHash()

dmitripivkine · 2026-04-23T15:36:47Z

+			if (J9_IS_J9CLASS_VALUETYPE(clazz)) {
+				J9VMThread *currentThread = vm->internalVMFunctions->currentVMThread(vm);
+				jboolean oomOccurred = JNI_FALSE;
+				_gcEnv.movedObjectHashCodeCache.originalHashCode = convertValueTypeToHash(vm, currentThread, objectPtr, clazz, &oomOccurred);


This code is much better now, and it is good enough.
However I wonder can jboolean oomOccurred notion to be excluded from GC code?
It is not belong here. And it is not used other than function can return 0 in the case of native OOM.
So, is it possible to create another wrapper function in VM hash code with (vm, currentThread, objectPtr, clazz) signature to hide OOM but return 0 instead?
I presume this oomOccurred parameter is necessary for some cases.

-Add test to verify hash code behaviour on value types with null-restricted fields. Handles both flattened and non-flattened cases. -Add test to verify hashcode behaviour on value types with identity fields. -Add test to verify reading hash code from object backfill when objects are moved. -Add test to validate value type hash code with null reference fields. Related: eclipse-openj9#23502 Signed-off-by: Aditi Srinivas M <Aditi.Srini@ibm.com>

- Add test for hash code on value types with null-restricted fields covering flattened and non-flattened cases. - Add test for hash code on value types with identity fields. - Add test to validate reading hash code from object backfill after move. - Add test for hash code on value types with null reference fields. Related: eclipse-openj9#23502 Signed-off-by: Aditi Srinivas M <Aditi.Srini@ibm.com>

theresa-m self-requested a review March 12, 2026 16:30

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

AditiS11 force-pushed the valuetypeHashCode branch 2 times, most recently from 29d6177 to 6f419cf Compare March 13, 2026 11:42

theresa-m suggested changes Mar 13, 2026

View reviewed changes

AditiS11 marked this pull request as draft March 16, 2026 12:06

AditiS11 force-pushed the valuetypeHashCode branch 3 times, most recently from 59a45e3 to 437659e Compare March 18, 2026 11:17

AditiS11 marked this pull request as ready for review March 18, 2026 12:22

AditiS11 mentioned this pull request Mar 18, 2026

Value Types: valhalla/valuetypes/MethodHandleTest.java #22648

Open

theresa-m suggested changes Mar 19, 2026

View reviewed changes

keithc-ca requested changes Mar 19, 2026

View reviewed changes

AditiS11 force-pushed the valuetypeHashCode branch from 437659e to c5f3c7e Compare March 20, 2026 18:20

AditiS11 force-pushed the valuetypeHashCode branch 2 times, most recently from 75c5db8 to 9aeaac9 Compare March 23, 2026 12:23

theresa-m reviewed Mar 23, 2026

View reviewed changes

AditiS11 mentioned this pull request Mar 23, 2026

Implement arrayInstanceIndexScale() and arrayInstanceBaseOffset() #23565

Merged

AditiS11 force-pushed the valuetypeHashCode branch 2 times, most recently from 168ca66 to 111a450 Compare March 23, 2026 18:13

keithc-ca requested changes Mar 23, 2026

View reviewed changes

dmitripivkine reviewed Mar 23, 2026

View reviewed changes

AditiS11 force-pushed the valuetypeHashCode branch from 111a450 to 01beea1 Compare March 24, 2026 12:37

dmitripivkine reviewed Mar 24, 2026

View reviewed changes

AditiS11 marked this pull request as draft March 25, 2026 11:37

keithc-ca mentioned this pull request Apr 8, 2026

Track and release memory allocated by JVM_dladdr() #23664

Merged

Aditi Srinivas M added 3 commits April 9, 2026 08:32

Address review comments

5bfb859

-Add function to skip valuetype hash calculation in getObjectHashCode() -Add the oomOccurred parameter to handle OOM.

Address review comments

07b34a6

-Modify getObjectHashCode() to udate the hash slot for valuetypes. -Update initializeHashSlot() to calculate the actual hash for valuetypes.

AditiS11 marked this pull request as draft April 9, 2026 16:07

AditiS11 force-pushed the valuetypeHashCode branch from 8d9b5a1 to d80a039 Compare April 13, 2026 14:17

Address review comments

fb1f64b

-Handle OOM when objectHashCode() is called -Add convertValueToHashForObjectWithForwardedHeader which accepts class as a parameter

AditiS11 force-pushed the valuetypeHashCode branch from d80a039 to fb1f64b Compare April 13, 2026 14:20

dmitripivkine reviewed Apr 13, 2026

View reviewed changes

dmitripivkine reviewed Apr 14, 2026

View reviewed changes

theresa-m suggested changes Apr 14, 2026

View reviewed changes

AditiS11 marked this pull request as ready for review April 16, 2026 15:23

theresa-m suggested changes Apr 20, 2026

View reviewed changes

AditiS11 marked this pull request as draft April 21, 2026 10:35

AditiS11 force-pushed the valuetypeHashCode branch 3 times, most recently from d754d28 to 2116a54 Compare April 22, 2026 13:27

Address review comments

147d3ad

- Overload the function convertValueToHashForObjecti() to take the J9Class structure from forwarded header -Remove inlineConvertValueObjectToHash()

AditiS11 force-pushed the valuetypeHashCode branch from 2116a54 to 147d3ad Compare April 23, 2026 14:52

dmitripivkine reviewed Apr 23, 2026

View reviewed changes

AditiS11 mentioned this pull request Apr 23, 2026

Add tests to verify value type hash code behaviour #23813

Open

		@@ -70,7 +70,6 @@ final class J9VMInternals {

		/[IF INLINE-TYPES]/
		static boolean positiveOnlyHashcodes = positiveOnlyHashcodes();

Conversation

AditiS11 commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AditiS11 commented Mar 12, 2026

Uh oh!

theresa-m left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AditiS11 Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

AditiS11 Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

theresa-m left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

theresa-m Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

AditiS11 commented Mar 12, 2026 •

edited

Loading

AditiS11 Mar 17, 2026 •

edited

Loading

AditiS11 Mar 17, 2026 •

edited

Loading

theresa-m left a comment •

edited

Loading

theresa-m Mar 20, 2026 •

edited

Loading