Use primitiveObjectHashCode function when hashing value types

[ehrenjulzert]

for #15529

Signed-off-by: Ehren Julien-Neitzert [email protected]

[ehrenjulzert]

@hangshao0

[hangshao0]

There is a DDR version of hashcode() implementation, it needs to be updated to check value type as well: https://github.com/eclipse-openj9/openj9/blob/f6383e2713197f77acb78c717bebce32d68ab27d/debugtools/DDR_VM/src/com/ibm/j9ddr/vm29/j9/gc/GCObjectModel_V1.java#L325

[ehrenjulzert]

There is a DDR version of hashcode() implementation

I see that this is just the GC/native hashcode logic rewritten in Java, which makes me think that it's just supposed to reflect exactly what's happening in ObjectModel.hpp and ObjectHash.hpp. Since my code is only implemented at the Java level it won't ever be called by GC. Despite this does it still make sense for me to update the DDR code?

[hangshao0]

There is a DDR command that returns the hashcode given an object address: https://github.com/eclipse-openj9/openj9/blob/master/debugtools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/HashCodeCommand.java#L41

The passed in object by the user could be either an identity object or a value type. The existing DDR hashcode implementation only works for identity object, so a check of identity object vs value type is needed there.

As in this PR, OpenJ9 does not implement the hashcode algorithm of value type itself (it uses an OpenJDK API to do so). The value type hashcode won't be cached with the object either, not sure we need DDR hashcode to support value type. If not, we need to at least update the DDR hashcode command saying it does not support value type and handle the error case if a value type is passed in.

@keithc-ca Any comments ?

[keithc-ca]

I think the right answer is to update the DDR code to recognize value types and simply respond that they don't have hashcodes.

[ehrenjulzert]

I added an error message to the DDR hashcode command in 6e5bf07 which says "cannot calculate the hashcode of a value type object"

[ehrenjulzert]

There is an issue I'm currently looking at where the MN_FLATTENED flag is never set in MemberName.java, which causes primitiveObjectHashCode to segfault when hashing flattened VTs. I will need to fix that first before this PR is merged

[ehrenjulzert]

The MN_FLATTENED issue has been fixed, this is ready for review now

[hangshao0]

You may want to add some testing code for this.

[ehrenjulzert]

Test added in bd69633 in ValueTypeTests.java

[hangshao0]

Test added in https://github.com/eclipse-openj9/openj9/commit/bd696339e6c06f77a6312a90126daec501c0e315 in ValueTypeTests.java

The tests there are using generated VT classes, which will be eventually replaced. You can add the new test in a new file similar to ValueTypeUnsafeTests.java and reuse the VT classes in ValueTypeUnsafeTestClasses.java. Probably it is better to rename ValueTypeUnsafeTestClasses.java to ValueTypeTestClasses.java.

I see there are only primitive value types in ValueTypeUnsafeTestClasses.java, you can add non-primitive VT there, i.e. classes define by value keyword, and test hashcode both on primitive and non-primitive VTs.

[ehrenjulzert]

Currently all the primitive classes in ValueTypeUnsafeTestClasses.java use the naming convention ValueType<classname>, so adding classes that actually use the value keyword might be confusing unless I rename all of the primitive classes to PrimitiveType<classname>. However changing all of those class names in both ValueTypeUnsafeTestClasses.java and ValueTypeUnsafeTests.java feels out of scope for this PR, so maybe I should make a new PR for testing the hashcode function?

I could instead name the new value type classes NonPrimitiveValueType<classname> to differentiate them from the primitive classes, but that seems like not as good a solution.

[hangshao0]

However changing all of those class names in both ValueTypeUnsafeTestClasses.java and ValueTypeUnsafeTests.java feels out of scope for this PR, so maybe I should make a new PR for testing the hashcode function?

I am fine with adding the test in a separate PR. You can decide.

I could instead name the new value type classes NonPrimitiveValueType to differentiate them from the primitive classes, but that seems like not as good a solution.

You can probably use ValueClass<className> for classes defined via keyword value.

[ehrenjulzert]

Okay, I'll take out the current test code and put it in another PR then

[hangshao0]

Test code is being added via https://github.com/eclipse-openj9/openj9/pull/16177.

[tajila]

Jenkins test sanity xlinuxvalst jdknext

[tajila]

Jenkins test sanity win jdk8

[tajila]

Jenkins test sanity,extended plinuxval jdknext

[tajila]

Jenkins test sanity,extended plinuxval jdknext

[tajila]

Jenkins test sanity plinuxvalst jdknext

[ehrenjulzert]

looks like the build failure is due to https://github.com/eclipse-openj9/openj9/issues/16028