Iterative implementation for isSubstitutable#23445
Iterative implementation for isSubstitutable#23445AditiS11 wants to merge 1 commit intoeclipse-openj9:masterfrom
Conversation
|
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. |
theresa-m
added
comp:vm
project:valhalla
|
|
||
| struct FieldComparisonFrame | ||
| { | ||
| j9object_t lhs; |
There was a problem hiding this comment.
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.
runtime/vm/ValueTypeHelpers.hpp
Outdated
| UDATA startOffset, | ||
| J9Class *clazz) | ||
| { | ||
| #if defined(J9VM_OPT_VALHALLA_VALUE_TYPES) |
There was a problem hiding this comment.
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.
runtime/vm/ValueTypeHelpers.hpp
Outdated
| 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); |
There was a problem hiding this comment.
You can directly return the result of compareValueType instead of creating a variable.
runtime/vm/ValueTypeHelpers.hpp
Outdated
| 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. |
There was a problem hiding this comment.
I think a BFS approach would make more sense for this method so you don't need to store iterators.
runtime/vm/ValueTypeHelpers.hpp
Outdated
| 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 was a problem hiding this comment.
There is a circular call here. acmp() calls isSubstitutable.
There was a problem hiding this comment.
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 was a problem hiding this comment.
There could still be many method calls added to the stack if an object field has another object field.
AditiS11
force-pushed
the
iterative_impl
branch
2 times, most recently
from
14eedce to
ab51a40
Compare
runtime/vm/ValueTypeHelpers.hpp
Outdated
| U_32 walkFlags = J9VM_FIELD_OFFSET_WALK_INCLUDE_INSTANCE; | ||
| J9ROMFieldOffsetWalkState state; | ||
|
|
||
| std::queue<FieldComparisonFrame> queue; |
There was a problem hiding this comment.
We do not use STL. You can implement a queue using an array.
AditiS11
force-pushed
the
iterative_impl
branch
from
ab51a40 to
0bd0a32
Compare
Replace recursive approach to traverse fields. Add a helper function 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
0bd0a32 to
885375f
Compare
| U_32 walkFlags = J9VM_FIELD_OFFSET_WALK_INCLUDE_INSTANCE; | ||
| J9ROMFieldOffsetWalkState state; | ||
|
|
||
| const UDATA MAX_QUEUE_SIZE = 512; |
There was a problem hiding this comment.
Defined the max queue size as 512. Not sure if this is sufficient to support the behaviour of valuetypes.
There was a problem hiding this comment.
512 should be sufficient.
3 participants
Replace recursive approach to traverse fields.
Add a helper function to iteratively traverse the fields.
Use DFS with a stack to handle nested flattened value types.
Related: #15768
Signed-off-by: Aditi Srinivas M Aditi.Srini@ibm.com