Iterative implementation for isSubstitutable by AditiS11 · Pull Request #23445 · eclipse-openj9/openj9

AditiS11 · 2026-03-04T13:14:13Z

Replace recursive approach to traverse fields.
Modify isSubstitutable to iteratively traverse the fields.
Use BFS with a queue to handle nested flattened value types.

Related: #15768

AditiS11 · 2026-03-04T13:33:30Z

Creating a single helper function for field iteration would require two-passes of the stack, first collecting all fields into a stack, then performing the comparison. I thought that this would increase the memory overhead.
The current approach performs comparison as fields are discovered during traversal.
Hence I thought of creating separate helper functions for isSubstitutable and hash code calculation, each maintaining its own stack for field traversal.

theresa-m · 2026-03-04T14:45:03Z

+
+	struct FieldComparisonFrame
+	{
+		j9object_t lhs;


The value of lhs and rhs appear to be the same for each entry so I don't think they need to be saved. state is not used.

theresa-m · 2026-03-04T14:49:24Z

+			UDATA startOffset,
+			J9Class *clazz)
+	{
+#if defined(J9VM_OPT_VALHALLA_VALUE_TYPES)


I think it would make more sense to move this flag around isSubstitutable. Your helper code should be under J9VM_OPT_VALHALLA_VALUE_TYPES as well.

theresa-m · 2026-03-04T14:51:00Z

+		Assert_VM_notNull(lhs);
+		Assert_VM_notNull(rhs);
+		Assert_VM_true(J9OBJECT_CLAZZ(currentThread, lhs) == J9OBJECT_CLAZZ(currentThread, rhs));
+		rc = compareValueType(currentThread, objectAccessBarrier, lhs, rhs, startOffset, clazz);


You can directly return the result of compareValueType instead of creating a variable.

theresa-m · 2026-03-04T19:43:14Z

-	static bool
-	isSubstitutable(J9VMThread *currentThread, MM_ObjectAccessBarrierAPI objectAccessBarrier, j9object_t lhs, j9object_t rhs, UDATA startOffset, J9Class *clazz)
+	 *  Iteratively traverse the fields and determine if they are equal.
+	 *  Use depth-first search with a stack to handle nested flattened value types.


I think a BFS approach would make more sense for this method so you don't need to store iterators.

theresa-m · 2026-03-04T19:51:48Z

-						goto done;
+						j9object_t lhsFieldObject = objectAccessBarrier.inlineMixedObjectReadObject(currentThread, frame.lhs, fieldOffset);
+						j9object_t rhsFieldObject = objectAccessBarrier.inlineMixedObjectReadObject(currentThread, frame.rhs, fieldOffset);
+						bool rc = VM_ValueTypeHelpers::acmp(currentThread, objectAccessBarrier, lhsFieldObject, rhsFieldObject);


There is a circular call here. acmp() calls isSubstitutable.

Although acmp() calls isSubstitutable(), it starts a new traversal frame on the referenced object. I think it will not go into an infinite recursion.

There could still be many method calls added to the stack if an object field has another object field.

I think it will not will not go into an infinite recursion

Please elaborate: Why are cycles involving value objects and non-value objects impossible?

In my previous approach, acmp() was called from isSubstitutable() to compare reference fields. isSubstitutable() had a check to push value type objects into a queue so that their fields could be compared.
Hence it wouldn't be an infinite recursion.

However in the current implementation I have removed the call to acmp() in isSubstitutable() completely as Theresa suggested.

hangshao0 · 2026-03-09T15:40:29Z

 		U_32 walkFlags = J9VM_FIELD_OFFSET_WALK_INCLUDE_INSTANCE;
-		J9ROMFieldOffsetWalkState state;
+
+		std::queue<FieldComparisonFrame> queue;


We do not use STL. You can implement a queue using an array.

AditiS11 · 2026-03-11T15:31:12Z

 		U_32 walkFlags = J9VM_FIELD_OFFSET_WALK_INCLUDE_INSTANCE;
-		J9ROMFieldOffsetWalkState state;
+
+		const UDATA MAX_QUEUE_SIZE = 512;


Defined the max queue size as 512. Not sure if this is sufficient to support the behaviour of valuetypes.

512 should be sufficient.

Why is this sufficient? There could still be any number of fields.

The description says Use DFS so I assumed it is the depth.

Seems that the implementation is changed to do BFS, so 512 is not guaranteed to be big enough.

You can start with an array on stack with a default initial size (like 128). In the case of overflow, you can switch to a new array (2X size of old array) allocated from malloc and copy the data into the new array.

I had suggested to switch to BFS since the DFS approach required saving much more information including field iterators in each queue entry. If the memory outlook for DFS is potentially better we can do that instead.

I don't have a strong preference between DFS or BFS.

theresa-m · 2026-03-12T16:11:26Z

 #include "ObjectAccessBarrierAPI.hpp"
 #include "VMHelpers.hpp"
 #include "vm_api.h"
+#include <queue>


This library is not used.

theresa-m

Please update the pull request description to reflect that this is now BFS.

theresa-m · 2026-03-13T13:38:34Z

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


There should be an out of memory error thrown if memory allocation fails. You can modify the return value to an integer to return different types of results like is done in copyFlattenableArray.

theresa-m · 2026-03-13T13:41:42Z

+						U_64 rhsValue = objectAccessBarrier.inlineMixedObjectReadU64(currentThread, frame.rhs, fieldOffset);
+
+						if (lhsValue != rhsValue) {
+							j9mem_free_memory(queue);


I suggest jumping to a label that will free the queue memory since this is done in a lot of places.

theresa-m · 2026-03-13T13:42:15Z

+						if (flattened) {
+							if (tail >= queueSize) {
+								UDATA newQueueSize = queueSize * 2;
+								FieldComparisonFrame *newQueue = (FieldComparisonFrame *)j9mem_allocate_memory(newQueueSize * sizeof(FieldComparisonFrame), OMRMEM_CATEGORY_VM);


I suggest creating a helper method to create and resize the queue since a lot of this code is duplicated.

theresa-m · 2026-03-16T17:30:44Z

+	 *  Use breadth-first search with a queue to handle nested flattened value types.
+	 *
+	 *  @param[in] currentThread the current thread
+	 *  @param[in] lhs the lhs object address


objectAccessBarrier is missing from the param list.

theresa-m · 2026-03-16T17:32:05Z

+			FieldComparisonFrame frame = queue[head++];
+			if (J9_ARE_ALL_BITS_SET(frame.clazz->classFlags, J9ClassCanSupportFastSubstitutability)) {
+				bool compare = objectAccessBarrier.structuralFlattenedCompareObjects(currentThread, frame.clazz, frame.lhs, frame.rhs, frame.offset);
+				if ( false == compare) {


Extra space between ( and f.

theresa-m · 2026-03-16T17:39:37Z

+		UDATA queueSize = initialQueueSize;
+		UDATA head = 0;
+		UDATA tail = 0;
+		FieldComparisonFrame *queue = (FieldComparisonFrame *)j9mem_allocate_memory(sizeof(FieldComparisonFrame) * queueSize, OMRMEM_CATEGORY_VM);


To avoid allocating unnecessary memory I think this queue should only be created if we run into a field that needs to be stored.

theresa-m · 2026-03-16T17:51:32Z

 		}
+done:
+		if (-1 == result) {
+			vm->internalVMFunctions->setHeapOutOfMemoryError(currentThread);


Since this is only called from the interpreter the stack will also need to be setup like https://github.com/eclipse-openj9/openj9/blob/master/runtime/vm/BytecodeInterpreter.hpp#L11718-L11723. I suggest to pass this error through acmp and handle the exception throwing in the interpreter instead.

theresa-m · 2026-03-17T16:49:31Z

+			_vm->internalVMFunctions->setHeapOutOfMemoryError(_currentThread);
+			rc = GOTO_THROW_CURRENT_EXCEPTION;
+			return rc;
+		}


This can be an else if. Same for line 8835. Also please compare cmp to its value in the next line since the result is no longer bool.

theresa-m · 2026-03-17T16:56:19Z

+		I_32 cmp = VM_ValueTypeHelpers::acmp(_currentThread, _objectAccessBarrier, lhs, rhs);
+		if (-1 == cmp) {
+			 _vm->internalVMFunctions->setHeapOutOfMemoryError(_currentThread);
+			 rc = GOTO_THROW_CURRENT_EXCEPTION;


You could use THROW_HEAP_OOM and follow the same pattern as other bytecodes by jumping to the line 8847 to return. Same for the other acmp use.

theresa-m · 2026-03-17T17:07:02Z

+			UDATA startOffset,
+			J9Class *clazz)
+	{
+		Assert_VM_notNull(lhs);


These asserts are redundant now since isSubstitutable is only called by acmp. I think compareValueType method can just be named isSubstitutable.

theresa-m · 2026-03-17T17:10:28Z

+					goto done;
 				}
-				case 'L': {
+				goto nextFrame;


I don't see this defined.

Sorry, I had added it in an earlier version, but I have now wrapped the entire field walk in the else block. Hence there is no need for the nextFrame label.

theresa-m · 2026-03-17T17:19:17Z

+		UDATA head = 0;
+		UDATA tail = 0;
+		FieldComparisonFrame *queue = NULL;
+		FieldComparisonFrame frame = { lhs, rhs, clazz, startOffset};


There is an extra space between { and l or a missing space between t and }.

theresa-m · 2026-03-17T17:20:24Z

+								flattened = J9_IS_FIELD_FLATTENED(fieldClass, walkResult->field);
+							}
+							if (flattened) {
+								if (queue == NULL) {


NULL == queue, same with 237.

theresa-m · 2026-03-18T13:38:37Z

 			rc = GOTO_BRANCH_WITH_ASYNC_CHECK;
+		} else if (-1 == cmp) {
+			rc = THROW_HEAP_OOM;
+			goto done;


Sorry I think my last review was confusing, this goto isn't needed.

theresa-m · 2026-03-18T13:39:22Z

 	 * Data members
 	 */
 private:
+	struct FieldComparisonFrame


This should be under J9VM_OPT_VALHALLA_VALUE_TYPES.

hangshao0 · 2026-03-19T13:51:15Z

-	JIT_RETURN_UDATA(currentThread->javaVM->internalVMFunctions->valueTypeCapableAcmp(currentThread, lhs, rhs));
+	I_32 cmp = currentThread->javaVM->internalVMFunctions->valueTypeCapableAcmp(currentThread, lhs, rhs);
+	if (-1 == cmp) {
+		JIT_RETURN_UDATA(setHeapOutOfMemoryErrorFromJIT(currentThread));


j9mem_allocate_memory() is native memory allocation, so this should be setNativeOutOfMemoryErrorFromJIT.

Similarly in BytecodeInterpreter.hpp, the j9mem_allocate_memory() allocation failure (from isSubstitutable()) should result in NativeOutOfMemoryError.

hangshao0 · 2026-03-19T14:11:16Z

There are some change to JIT helpers in cnathelp.cpp here. FYI @a7ehuo @hzongaro

keithc-ca · 2026-03-20T17:17:47Z

Please use parentheses here and in the commit message to clarify that the references is to a function: isSubstitutable().

hangshao0 · 2026-03-31T15:04:17Z

Any more review comments ? @keithc-ca

keithc-ca · 2026-03-31T15:26:43Z

I'll have another look at this later today.

hangshao0 · 2026-03-31T17:39:08Z

@hzongaro @a7ehuo Do you have a chance to review the JIT helper changes here ?

a7ehuo · 2026-03-31T18:07:05Z

@a7ehuo Do you have a chance to review the JIT helper changes here ?

Thanks for checking in. Sorry, unfortunately, I haven't had a chance to review this change yet. It would probably be better to have @hzongaro take a look instead.

hangshao0 · 2026-04-06T14:30:50Z

@keithc-ca @hzongaro Review comments ?

keithc-ca

This should be rebased to avoid implicit merges for

runtime/oti/j9nonbuilder.h
runtime/oti/vm_api.h
runtime/vm/BytecodeInterpreter.hpp

Replace recursive approach to traverse fields. Modify isSubstitutable() to iteratively traverse the fields. Use BFS with a queue to handle nested flattened value types. Related: eclipse-openj9#15768 Signed-off-by: Aditi Srinivas M <Aditi.Srini@ibm.com>

AditiS11 force-pushed the iterative_impl branch from efc183f to 18ef4a8 Compare March 4, 2026 13:17

theresa-m self-requested a review March 4, 2026 13:59

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

theresa-m suggested changes Mar 6, 2026

View reviewed changes

AditiS11 force-pushed the iterative_impl branch 2 times, most recently from 14eedce to ab51a40 Compare March 9, 2026 12:10

hangshao0 reviewed Mar 9, 2026

View reviewed changes

AditiS11 force-pushed the iterative_impl branch 2 times, most recently from 0bd0a32 to 885375f Compare March 11, 2026 15:30

AditiS11 commented Mar 11, 2026

View reviewed changes

theresa-m suggested changes Mar 12, 2026

View reviewed changes

AditiS11 force-pushed the iterative_impl branch from 885375f to 891a23d Compare March 13, 2026 11:44

theresa-m suggested changes Mar 13, 2026

View reviewed changes

AditiS11 force-pushed the iterative_impl branch 2 times, most recently from 82e4078 to 11dd004 Compare March 16, 2026 12:32

theresa-m suggested changes Mar 16, 2026

View reviewed changes

AditiS11 force-pushed the iterative_impl branch 4 times, most recently from 4578055 to 9309b5d Compare March 17, 2026 16:09

theresa-m reviewed Mar 17, 2026

View reviewed changes

AditiS11 force-pushed the iterative_impl branch 2 times, most recently from 5357d9b to 7364525 Compare March 18, 2026 11:19

theresa-m suggested changes Mar 18, 2026

View reviewed changes

AditiS11 force-pushed the iterative_impl branch from 7364525 to 9bcda6e Compare March 18, 2026 15:12

theresa-m approved these changes Mar 18, 2026

View reviewed changes

theresa-m requested a review from hangshao0 March 18, 2026 15:19

hangshao0 reviewed Mar 18, 2026

View reviewed changes

Comment thread runtime/vm/ValueTypeHelpers.hpp Outdated

Comment thread runtime/vm/ValueTypeHelpers.hpp Outdated

Comment thread runtime/vm/ValueTypeHelpers.hpp

AditiS11 force-pushed the iterative_impl branch from 9bcda6e to 47097c1 Compare March 19, 2026 12:52

hangshao0 reviewed Mar 19, 2026

View reviewed changes

hangshao0 requested review from a7ehuo and hzongaro March 19, 2026 14:11

keithc-ca requested changes Mar 19, 2026

View reviewed changes

Comment thread runtime/vm/ValueTypeHelpers.hpp Outdated

Comment thread runtime/vm/ValueTypeHelpers.hpp Outdated

Comment thread runtime/vm/ValueTypeHelpers.hpp Outdated

Comment thread runtime/vm/ValueTypeHelpers.hpp Outdated

AditiS11 force-pushed the iterative_impl branch from 47097c1 to fd62634 Compare March 20, 2026 11:57

hangshao0 approved these changes Mar 20, 2026

View reviewed changes

keithc-ca requested changes Mar 20, 2026

View reviewed changes

AditiS11 force-pushed the iterative_impl branch from fd62634 to 4cdcbad Compare March 20, 2026 16:35

AditiS11 force-pushed the iterative_impl branch 2 times, most recently from 901fdd8 to 96634be Compare March 23, 2026 18:00

keithc-ca requested changes Mar 23, 2026

View reviewed changes

Comment thread runtime/codert_vm/cnathelp.cpp Outdated

Comment thread runtime/vm/ValueTypeHelpers.hpp Outdated

Comment thread runtime/vm/ValueTypeHelpers.hpp Outdated

AditiS11 force-pushed the iterative_impl branch from 96634be to 3cea4ae Compare March 24, 2026 06:44

keithc-ca requested changes Mar 24, 2026

View reviewed changes

Comment thread runtime/codert_vm/cnathelp.cpp Outdated

Comment thread runtime/codert_vm/cnathelp.cpp Outdated

Comment thread runtime/codert_vm/cnathelp.cpp Outdated

AditiS11 force-pushed the iterative_impl branch from 3cea4ae to ce3853e Compare March 25, 2026 06:27

keithc-ca requested changes Mar 25, 2026

View reviewed changes

Comment thread runtime/vm/ValueTypeHelpers.hpp Outdated

Comment thread runtime/vm/ValueTypeHelpers.hpp Outdated

AditiS11 force-pushed the iterative_impl branch from ce3853e to a5eef68 Compare March 26, 2026 12:31

AditiS11 requested a review from keithc-ca March 30, 2026 12:40

keithc-ca requested changes Mar 31, 2026

View reviewed changes

Comment thread runtime/vm/ValueTypeHelpers.hpp Outdated

AditiS11 force-pushed the iterative_impl branch from a5eef68 to e3207ea Compare April 1, 2026 13:36

keithc-ca requested changes Apr 6, 2026

View reviewed changes

Comment thread runtime/vm/ValueTypeHelpers.hpp Outdated

AditiS11 force-pushed the iterative_impl branch from e3207ea to c599849 Compare April 7, 2026 07:30

Conversation

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

Uh oh!

Uh oh!

AditiS11 commented Mar 4, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AditiS11 commented Mar 4, 2026 •

edited

Loading

hangshao0 commented Mar 31, 2026 •

edited

Loading