Fix null restricted array related issues for value types #20112

a7ehuo · 2024-09-04T18:07:53Z

(1) Add utility methods for null restricted arrays
(2) Fix null restricted array related issues

Add recognized method jdk/internal/value/ValueClass.newArrayInstance
Add VP fixed class constraint if ValueClass.newArrayInstance creates null restricted array
Rename isArrayCompTypePrimitiveValueType to isArrayNullRestricted to reflect the updated logic
Update isArrayNullRestricted on how to determine whether or not an array is a null restricted array
Disable transformation based on type hint which is no longer sufficient enough to decide whether or not the array is null restricted or not
If flattenable arrays are enabled, do not create fixed class constraint for parm array class based on signature since both nullable and null restricted arrays share the same signature

Depends on

Fixes: #19913, #19914, #19708

a7ehuo · 2024-09-04T18:29:48Z

@hzongaro May I ask you to review this change? Thank you very much!

This PR is created as a draft and marked as WIP for now because of all the dependencies. Regarding to the implementation itself, it is ready for review.

Since this PR depends on eclipse/omr#7452, it should be review along with eclipse/omr#7452.

hzongaro

I think these changes look good - just a few minor comments.

Could you expand on the reason for these changes in the commit comment? May I also ask you to remove the Markdown from the commit comment?

runtime/oti/j9.h

runtime/compiler/env/VMJ9.h

runtime/compiler/env/VMJ9.cpp

runtime/compiler/env/J9ClassEnv.cpp

runtime/compiler/env/VMJ9Server.cpp

hzongaro

Sorry - I somehow neglected to review the Value Propagation changes in my first pass through this pull request

runtime/compiler/optimizer/J9ValuePropagation.cpp

runtime/compiler/env/j9method.cpp

runtime/compiler/optimizer/J9ValuePropagation.cpp

a7ehuo · 2024-10-08T15:41:26Z

@hzongaro All comments have been addressed in the latest commits, except for the following three that will be addressed in the future PRs due to the complexity. Ready for another review. Thank you!

(1) #20112 (comment)
(2) #20112 (comment)
(3) #20112 (comment)

Please note that I have rebased this PR to the latest master branch and this PR requires coordinated merge with eclipse/omr#7452.

hzongaro

Thanks for the updates! Just a few more comments.

runtime/compiler/env/j9method.cpp

runtime/compiler/optimizer/J9ValuePropagation.cpp

runtime/compiler/runtime/SymbolValidationManager.cpp

Whether or not an array is null-restricted is no longer decided by array component type but by the J9ClassArrayIsNullRestricted flag in array class. Only the nullRestrictedArrayClass of the J9Class has J9ClassArrayIsNullRestricted set. (1) Add getNullRestrictedArrayClassFromComponentClass to retrieve nullRestrictedArrayClass from J9Class (2) Add isArrayNullRestricted to check if an array class is null restricted by checking J9ClassArrayIsNullRestricted. Signed-off-by: Annabelle Huo <[email protected]>

a7ehuo · 2024-10-11T22:36:33Z

@hzongaro All comments are addressed. Ready for another review. Thank you!

I rebased the code in order to resolve the merge conflict in CommunicationStream.hpp

hzongaro

Thanks for the updates! Just a couple of more minor comments.

runtime/compiler/optimizer/J9ValuePropagation.cpp

(1) Add recognized methods from jdk/internal/value/ValueClass and jdk/internal/value/NullRestrictedCheckedType (2) Add VP fixed class constraint if ValueClass.newArrayInstance creates null restricted array using NullRestrictedCheckedType (3) Rename isArrayCompTypePrimitiveValueType to isArrayNullRestricted to reflect the updated logic Update isArrayNullRestricted on how to determine whether or not an array is a null restricted array (4) Disable transformation based on type hint which is no longer sufficient enough to decide whether or not the array is null restricted or not (5) If flattenable arrays are enabled, do not create fixed class constraint for parm array class based on signature since both nullable and null restricted arrays share the same signature (6) Enable previous disabled JIT Valhalla functional tests Fixes: eclipse-openj9#19913, eclipse-openj9#19914, eclipse-openj9#19708 Signed-off-by: Annabelle Huo <[email protected]>

a7ehuo · 2024-10-15T16:47:24Z

@hzongaro All comments are addressed. Ready for another review. Thank you!

hzongaro

Looks good. Thanks!

hzongaro · 2024-10-15T18:33:30Z

Jenkins test sanity.functional,extended.functional xlinuxval,plinuxval,zlinuxval,alinuxval jdknext depends eclipse/omr#7452

hzongaro · 2024-10-15T18:34:24Z

Jenkins test sanity.functional,sanity.openjdk xlinuxvalst,plinuxvalst,zlinuxvalst,alinuxvalst jdknext depends eclipse/omr#7452

hzongaro · 2024-10-15T18:41:40Z

Jenkins test sanity.functional,sanity.openjdk all jdk8,jdk11,jdk17,jdk21 depends eclipse/omr#7452

a7ehuo · 2024-10-16T13:20:35Z

Test_openjdk11_j9_sanity.openjdk_aarch64_linux_Personal: Known issue #13756

[2024-10-15T20:03:29.491Z] java.lang.Exception
[2024-10-15T20:03:29.491Z] 	at ReplayCacheTestProc.main0(ReplayCacheTestProc.java:279)
[2024-10-15T20:03:29.491Z] 	at ReplayCacheTestProc.main(ReplayCacheTestProc.java:326)
[2024-10-15T20:03:29.491Z] 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
[2024-10-15T20:03:29.491Z] 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
[2024-10-15T20:03:29.491Z] 	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
[2024-10-15T20:03:29.491Z] 	at java.base/java.lang.reflect.Method.invoke(Method.java:572)
[2024-10-15T20:03:29.491Z] 	at com.sun.javatest.regtest.agent.MainWrapper$MainTask.run(MainWrapper.java:138)
[2024-10-15T20:03:29.491Z] 	at java.base/java.lang.Thread.run(Thread.java:839)
[2024-10-15T20:03:29.491Z] java.lang.Exception
[2024-10-15T20:03:29.491Z] 	at ReplayCacheTestProc.main0(ReplayCacheTestProc.java:279)
[2024-10-15T20:03:29.491Z] 	at ReplayCacheTestProc.main(ReplayCacheTestProc.java:326)
...
[2024-10-15T20:04:55.398Z] jdk_security4_0_FAILED

Test_openjdk11_j9_sanity.openjdk_x86-64_windows_Personal: Known issue #17743

[2024-10-16T02:47:35.873Z] java.lang.RuntimeException: PortUnreachableException not thrown.
[2024-10-16T02:47:35.873Z] 	at Unreachable.main(Unreachable.java:102)
...
[2024-10-16T02:52:31.349Z] jdk_security4_1_FAILED

Test_openjdk17_j9_sanity.openjdk_ppc64_aix_Personal: Known issue: #19962

[2024-10-15T22:10:03.705Z] ACTION: main -- Error. Agent communication error: java.io.EOFException; check console log for any additional details
[2024-10-15T22:10:03.705Z] REASON: Assumed action based on file name: run main TestCipherMode 
...
[2024-10-15T22:15:02.584Z] jdk_security2_1_FAILED

Test_openjdk17_j9_sanity.openjdk_x86-64_windows_Personal: Known issue #18708

[2024-10-16T01:51:10.631Z] Caused by: java.lang.OutOfMemoryError: Java heap space
[2024-10-16T01:51:10.631Z] 	at java.base/java.lang.String.repeat(String.java:5202)
...
[2024-10-16T02:02:18.562Z] jdk_util_0_FAILED

Test_openjdk8_j9_sanity.functional_x86-64_linux_Personal: Known issue: #19224

[2024-10-15T22:44:29.181Z]  [ERR] /home/jenkins/workspace/Test_openjdk8_j9_sanity.functional_x86-64_linux_Personal_testList_1/aqa-tests/TKG/../../jvmtest/functional/cmdLineTests/jitserver/jitserverconfig.sh: line 30: lsof: command not found
...
[2024-10-15T22:44:29.181Z] testJitserverArguments_0_FAILED

Valhalla test passes on x86-64 which is what I have tested locally. Valhalla on other platforms (Power, Z, aarch64) fail, which I will take a look at next.

I'm not sure about vt_standard tests yet

hzongaro · 2024-10-16T13:34:21Z

I took a quick look at the sanity.functional and sanity.openjdk "vt-standard" testing. It looks like those tests have seen many failures in recent runs. For instance, an aarch64 Linux sanity.functional vt-standard PR test run from earlier this month yielded 55 failures.

We'll need to do further investigation, but it seems unlikely that this pull request is responsible for the failures. These failures will not hold back merging this pull request.

a7ehuo · 2024-10-16T20:06:15Z

Valhalla test passes on x86-64 which is what I have tested locally. Valhalla on other platforms (Power, Z, aarch64) fail, which I will take a look at next.

Two main tests fail on Power, Z, and Aarch64: ValueTypeArrayTestsJIT and ValueTypeSystemArraycopyTests.

I looked at the failure in ValueTypeArrayTestsJIT first. The failure is related to how instanceOf is evaluated on Power, Z, and Aarch64. On x86, we call helper jitInstanceOf which explains why it passes.

It failed because instanceOf doesn't think null restricted array PointPV[] is an instance of [Lorg/openj9/test/lworld/ValueTypeArrayTests$PointPV; in ValueTypeArrayTests.runTest

[ValueTypeArrayTests] [INFO] calling runTest testArrays[3] NullRestrictedArray nullObj kinds[0] OBJ_TYPE 1 kinds[0] OBJ_TYPE 1
[ValueTypeArrayTests] [INFO] --------------------------
[ValueTypeArrayTests] [INFO] runTest staticArrayKind=OBJ_TYPE 1 staticSourceKind=OBJ_TYPE 1
[ValueTypeArrayTests] [INFO] runTest arr: actualArrayKind=IFACE_TYPE 2 sourceVal: actualSourceKind=NULL_REF 0 //<--- wrong actualArrayKind should be PRIM_TYPE 4 since testArrays[3] is NullRestrictedArray
[ValueTypeArrayTests] [INFO] runTest expectedExceptionClass null
[ValueTypeArrayTests] [INFO] assignDispatch arrKind=OBJ_TYPE srcKind=OBJ_TYPE idx=1
[ValueTypeArrayTests] [INFO] runTest caughtThrowable=java.lang.ArrayStoreException

hzongaro · 2024-10-17T14:27:53Z

I looked at the failure in ValueTypeArrayTestsJIT first. The failure is related to how instanceOf is evaluated on Power, Z, and Aarch64. On x86, we call helper jitInstanceOf which explains why it passes.

It failed because instanceOf doesn't think null restricted array PointPV[] is an instance of [Lorg/openj9/test/lworld/ValueTypeArrayTests$PointPV; in ValueTypeArrayTests.runTest

Thanks, @a7ehuo! Should the relevant tests still be disabled and an issue opened, or do you plan to try to fix this problem in this pull request?

a7ehuo · 2024-10-17T14:56:21Z

I plan to disable the related two tests (ValueTypeArrayTestsJIT, ValueTypeSystemArraycopyTests) for now and open new PRs when ready.

There are already issues opened on them. I think I will reuse them for future submission. I will make a note in these two issues that this PR doesn't fix them on all hardware.
#19913
#19914

I'll run some Jenkins Valhalla tests on all platforms before pushing another commit

a7ehuo added comp:jit project:valhalla Used to track Project Valhalla related work depends:omr Pull request is dependent on a corresponding change in OMR depends:omr:breaking labels Sep 4, 2024

a7ehuo requested a review from hzongaro September 4, 2024 18:12

a7ehuo force-pushed the PR-valuetypes-null-restricted-array branch 2 times, most recently from c469e30 to 516a676 Compare September 4, 2024 18:21

a7ehuo mentioned this pull request Sep 4, 2024

Fix null restricted array related issues for value types eclipse/omr#7452

Open

1 task

hzongaro reviewed Sep 9, 2024

View reviewed changes

theresa-m mentioned this pull request Sep 9, 2024

Always create nullable array before null-restricted #20139

Merged

theresa-m mentioned this pull request Sep 18, 2024

Remove Q type checks in Valhalla code #18157

Open

a7ehuo mentioned this pull request Oct 3, 2024

Value Types: java.lang.ArrayStoreException with null-restricted array with -Xint #20223

Closed

a7ehuo force-pushed the PR-valuetypes-null-restricted-array branch 2 times, most recently from bb24a35 to 3af4ff9 Compare October 8, 2024 15:33

a7ehuo marked this pull request as ready for review October 8, 2024 15:34

a7ehuo requested a review from dsouzai as a code owner October 8, 2024 15:34

a7ehuo changed the title ~~WIP: Fix null restricted array related issues for value types~~ Fix null restricted array related issues for value types Oct 8, 2024

hzongaro reviewed Oct 11, 2024

View reviewed changes

a7ehuo force-pushed the PR-valuetypes-null-restricted-array branch 5 times, most recently from d9a0a5c to de9f0d5 Compare October 11, 2024 22:19

a7ehuo force-pushed the PR-valuetypes-null-restricted-array branch from de9f0d5 to 135b856 Compare October 11, 2024 22:32

a7ehuo force-pushed the PR-valuetypes-null-restricted-array branch from 135b856 to 6782be9 Compare October 11, 2024 22:40

hzongaro reviewed Oct 15, 2024

View reviewed changes

runtime/compiler/optimizer/J9ValuePropagation.cpp Outdated Show resolved Hide resolved

runtime/compiler/optimizer/J9ValuePropagation.cpp Outdated Show resolved Hide resolved

a7ehuo force-pushed the PR-valuetypes-null-restricted-array branch from 6782be9 to cc811af Compare October 15, 2024 16:46

hzongaro approved these changes Oct 15, 2024

View reviewed changes

hzongaro mentioned this pull request Oct 17, 2024

java.lang.IllegalAccessException thrown for jdk.internal.misc.Unsafe.theUnsafe in Valhalla vt_standard testing #20372

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix null restricted array related issues for value types #20112

Fix null restricted array related issues for value types #20112

a7ehuo commented Sep 4, 2024 •

edited

Loading

a7ehuo commented Sep 4, 2024

hzongaro left a comment

hzongaro left a comment

a7ehuo commented Oct 8, 2024

hzongaro left a comment

a7ehuo commented Oct 11, 2024

hzongaro left a comment

a7ehuo commented Oct 15, 2024

hzongaro left a comment

hzongaro commented Oct 15, 2024

hzongaro commented Oct 15, 2024

hzongaro commented Oct 15, 2024

a7ehuo commented Oct 16, 2024

hzongaro commented Oct 16, 2024 •

edited

Loading

a7ehuo commented Oct 16, 2024

hzongaro commented Oct 17, 2024

a7ehuo commented Oct 17, 2024

Fix null restricted array related issues for value types #20112

Are you sure you want to change the base?

Fix null restricted array related issues for value types #20112

Conversation

a7ehuo commented Sep 4, 2024 • edited Loading

a7ehuo commented Sep 4, 2024

hzongaro left a comment

Choose a reason for hiding this comment

hzongaro left a comment

Choose a reason for hiding this comment

a7ehuo commented Oct 8, 2024

hzongaro left a comment

Choose a reason for hiding this comment

a7ehuo commented Oct 11, 2024

hzongaro left a comment

Choose a reason for hiding this comment

a7ehuo commented Oct 15, 2024

hzongaro left a comment

Choose a reason for hiding this comment

hzongaro commented Oct 15, 2024

hzongaro commented Oct 15, 2024

hzongaro commented Oct 15, 2024

a7ehuo commented Oct 16, 2024

hzongaro commented Oct 16, 2024 • edited Loading

a7ehuo commented Oct 16, 2024

hzongaro commented Oct 17, 2024

a7ehuo commented Oct 17, 2024

a7ehuo commented Sep 4, 2024 •

edited

Loading

hzongaro commented Oct 16, 2024 •

edited

Loading