Add value type hash logic under Valhalla flag

[AditiS11]

• Guard zero hashcode check with Valhalla flags.

[dmitripivkine]

GC part of the change is good. And most likely there is the reason for regression. Need to inline or call function might change registers layout potentially.

[hangshao0]

Why not also adding a check if the object is VT before calling identityHashCode() ?

[AditiS11]

I felt that if the hash is zero, it's clear that the object is a value type, so I didn't add an additional check.

If we do add one, it introduces another if condition that would be executed. Could that cause a performance regression in Valhalla builds?

[hangshao0]

I felt that if the hash is zero, it's clear that the object is a value type, so I didn't add an additional check.

What this PR does is to move the zero hashcode case into the Valhalla flag, and it fixed a performance regression in non-Valhalla build, where there is no value types. So it means there are non-value types that have zero hashcode and they have a measurable performance impact if the code directly calls into identityHashCode(). Do I miss anything ?

[AditiS11]

The zero hashcode check was added by me while implementing hash calculation for value types.
So I think non-value types will not have a zero hash.

[hangshao0]

So I think non-value types will not have a zero hash.

Do we have code that prevent non-value type having zero hash ?

[hangshao0]

My point is that the regression (this PR is trying to fix) is caused by non-value type objects that have zero hash code and we repeated calls into identityHashCode() for sub objects. So we do have non-value type objects with zero hash.

[dmitripivkine]

My point is that the regression (this PR is trying to fix) is caused by non-value type objects that have zero hash code and we repeated calls into identityHashCode() for sub objects. So we do have non-value type objects with zero hash.

Do we have confirmation of this theory, do we really see valid 0 hash codes in mass for non-vt builds?
My theory most likely the reason of regression is adding handling of hash code equal 0 (calling function in this case) has introduced new registers/memory usage layout, for example move some values from registers to c-stack. This known xLC for Z compiler behaviour. As a result hot code in GC executes slowly.

[keithc-ca]

If value classes can be detected less expensively than an unnecessary call to identityHashCode(), then perhaps we should do that:

				/* If the hash is 0, and it is a value object, the hash should be calculated. */
				if ((0 == hashValue) && anObject.getClass().isValue()) {
					return identityHashCode(anObject);
				}

[hangshao0]

Do we have confirmation of this theory, do we really see valid 0 hash codes in mass for non-vt builds?

We all agree that the handling of zero hash should be moved inside the valhalla flags.

It is just whether to add anObject.getClass().isValue() check. @AditiS11 You can provide 2 builds with and without this check to the perf team to do a comparison.

[dmitripivkine]

As a test we can add print or even assertion if non-vt build discovers zero hash code. I have a suspicion we will never observe it.

[AditiS11]

I have added the isValue() check.
The check is within the Valhalla flag, hence I don't think it will create any difference in performance for the normal build.
Should I check the performance results a the Valhalla build?

[hangshao0]

I have added the isValue() check.
The check is within the Valhalla flag, hence I don't think it will create any difference in performance for the normal build.
Should I check the performance results a the Valhalla build?

@dmitripivkine suggested the following:

As a test we can add print or even assertion if non-vt build discovers zero hash code. I have a suspicion we will never observe it.

You can build a non-vt JDK with the suggested change and let the perf team run that to see if we really hit the non-VT zero hashcode case.

[AditiS11]

I have made the changes and shared the build with the perf team.

[hangshao0]

I have made the changes and shared the build with the perf team.

Thanks. Please update here once you get the result from the perf team.

[AditiS11]

Went through the benchmark console logs, the print statement was not hit.

[hangshao0]

Went through the benchmark console logs, the print statement was not hit.

OK, so we didn't not hit the zero hashcode case. I'll leave it to you and other reviewers whether to add/keep the anObject.getClass().isValue() check.

[AditiS11]

I have removed the anObject.getClass().isValue() check as I feel it would add a performance overhead for the valhalla builds.

[hangshao0]

@keithc-ca This is waiting for your approval.

[keithc-ca]

I'll make time to have another look tomorrow.

[keithc-ca]

@hangshao0 Please initiate testing you think appropriate.

[hangshao0]

Jenkins test sanity.functional,extended alinuxval jdknext

[hangshao0]

Jenkins compile amac jdk21