Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add methods to check if array class can be trusted as fixed class #20853

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add methods to check if array class can be trusted as fixed class #20853

wants to merge 1 commit into from

Conversation

a7ehuo
Copy link
Contributor

@a7ehuo a7ehuo commented Dec 18, 2024 •
edited
Loading

If null-restricted array is enabled and the class is an array class, the null-restricted array class and the nullable array class share the same signature. The null-restricted array can be viewed as a sub-type of the nullable array. Therefore, the constraint cannot be fixed class.

Related: #20522

@a7ehuo a7ehuo requested a review from hzongaro December 18, 2024 16:17
@a7ehuo a7ehuo added comp:jit project:valhalla Used to track Project Valhalla related work labels Dec 18, 2024
@a7ehuo a7ehuo mentioned this pull request Dec 18, 2024
Merged
@a7ehuo
Copy link
Contributor Author

a7ehuo commented Dec 18, 2024

The upstream APIs are created in eclipse-omr/omr#7579. @hzongaro May I ask you to review this change? Thank you!

hzongaro
hzongaro reviewed Dec 19, 2024
View reviewed changes
Copy link
Member

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the changes look correct. Just one suggestion.

runtime/compiler/optimizer/J9ValuePropagation.hpp Outdated Show resolved Hide resolved
@a7ehuo a7ehuo force-pushed the valuetype-fix-vp-constraint-for-nullRestrictedArray branch from 5964e8c to da631ba Compare December 19, 2024 18:21
hzongaro
hzongaro reviewed Dec 19, 2024
View reviewed changes
Copy link
Member

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I ask you to provide a bit more information in the commit comment about the circumstances in which these changes apply? Perhaps you can copy some of the comments you currently have in the upstream OMR commit. Also, the first line of the commit comment is 73 characters long. Per the commit guidelines, it should be less than 70 columns.

@a7ehuo
Add methods to check if array class can be trusted as fixed class
4b5152e 
If null-restricted array is enabled and the class is an array class,
the null-restricted array class and the nullable array class share
the same signature. The null-restricted array can be viewed
as a sub-type of the nullable array. Therefore, the constraint cannot
be fixed class.

Related: eclipse-openj9#20522
Signed-off-by: Annabelle Huo <[email protected]>
@a7ehuo a7ehuo force-pushed the valuetype-fix-vp-constraint-for-nullRestrictedArray branch from da631ba to 4b5152e Compare December 19, 2024 19:24
@a7ehuo a7ehuo changed the title Add utility methods to check if array class can be trusted as fixed class Add methods to check if array class can be trusted as fixed class Dec 19, 2024
@a7ehuo
Copy link
Contributor Author

a7ehuo commented Dec 19, 2024

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

hzongaro
hzongaro approved these changes Dec 19, 2024
View reviewed changes
Copy link
Member

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks! I will wait until the corresponding OMR changes have been promoted before running testing and merging this change.

@hzongaro hzongaro added the depends:omr Pull request is dependent on a corresponding change in OMR label Dec 19, 2024
@hzongaro
Copy link
Member

Jenkins test sanity.functional,sanity.openjdk xlinux,plinux jdk8,jdk17,jdk23

@hzongaro
Copy link
Member

Jenkins test sanity.functional xlinuxval jdknext

@hzongaro
Copy link
Member

Jenkins test sanity.functional xlinuxvalst jdknext

@hzongaro hzongaro self-assigned this Dec 21, 2024
@hzongaro
Copy link
Member

I'm running internal testing of the x86-64_linux_vt_standard to see whether the failures that were seen are seen without these changes.

@hzongaro
Copy link
Member

Jenkins test sanity.functional xlinuxvalst jdknext

@a7ehuo
Copy link
Contributor Author

a7ehuo commented Dec 24, 2024

There are 3 failed tests in the latest x86-64_linux_vt_standard test :

thrstatetest_0 and thrstatetest_1: Seems related to #20835

[2024-12-24T19:49:58.792Z] 306/306 Tests passed. 0 Failures ignored. 123 Failures expected.
[2024-12-24T19:49:58.792Z] ALL TESTS COMPLETED AND PASSED
[2024-12-24T19:50:00.012Z] Failed to destroy JVM
[2024-12-24T19:50:00.012Z] -----------------------------------
[2024-12-24T19:50:00.012Z] thrstatetest_1_FAILED
[2024-12-24T19:50:00.012Z] -----------------------------------

testJITServer_2: Seems the same as #14706

[2024-12-24T18:56:50.790Z] Failed to properly start server, it terminated prematurely with exit value: 1
[2024-12-24T18:56:50.790Z] Dumping the contents of log file: /home/jenkins/workspace/Test_openjdknext_j9_sanity.functional_x86-64_linux_vt_standard_Personal_testList_1/aqa-tests/TKG/output_17350614082637/testJITServer_2/testServerGoesDown.server.out
[2024-12-24T18:56:50.790Z] ////////////////////////////////////////////////////////////////////////
[2024-12-24T18:56:50.790Z] //// can't bind server address: Address already in use
[2024-12-24T18:56:50.790Z] //// 
[2024-12-24T18:56:50.790Z] //// JITServer is ready to accept incoming requests
[2024-12-24T18:56:50.790Z] //// Failed to open server socket on port 36862
[2024-12-24T18:56:50.790Z] ////////////////////////////////////////////////////////////////////////
...
[2024-12-24T19:01:55.597Z] 
[2024-12-24T19:01:55.597Z] -----------------------------------
[2024-12-24T19:01:55.597Z] testJITServer_2_FAILED
[2024-12-24T19:01:55.597Z] -----------------------------------

However, the previous x86-64_linux_vt_standard failed in building test when running javac. I could reproduce this issue locally. The failure is intermittent. I'm not sure whether or not this failure is related to this PR yet. I'll take a look.

    [javac] ----------- Stack Backtrace -----------
    [javac] getStringUTF8Length+0xb2 (0x00007FEFA6A69542 [libj9vm29.so+0x8a542])
    [javac] getStringUTFChars+0x3f (0x00007FEFA6A3491F [libj9vm29.so+0x5591f])
    [javac] Java_jdk_internal_loader_NativeLibrary_findEntry0+0x2f (0x00007FEF9481CC7F [libjava.so+0xcc7f])
    [javac]  (0x00007FEF2A05EFE5 [<unknown>+0x0])
    [javac] ---------------------------------------

@hzongaro Thank you for running all the tests! Let's hold off merging this PR for now. I'll take a look at the above crash first.

@a7ehuo
Copy link
Contributor Author

a7ehuo commented Dec 24, 2024

@hzongaro I'm able to reproduce the crash in getStringUTF8Length without this PR. Meanwhile I see this issue was just reported in #20870

/root/home/ahuo/src/openj9-openjdk-jdk.valuetypes/build/linux-x86_64-server-release/images/jdk/bin/java -version
openjdk version "24-internal" 2025-03-18
OpenJDK Runtime Environment (build 24-internal-adhoc.root.openj9-openjdk-jdk.valuetypes)
Eclipse OpenJ9 VM (build HEAD-9838ac7fa8, JRE 24 Linux amd64-64-Bit Compressed References 20241224_000000 (JIT enabled, AOT enabled)
OpenJ9   - 9838ac7fa8
OMR      - 3ddaad3c1
JCL      - 5fb3275617a based on jdk-24+26)

    [javac] ----------- Stack Backtrace -----------
    [javac] getStringUTF8Length+0xb2 (0x00007FEFA6A69542 [libj9vm29.so+0x8a542])
    [javac] getStringUTFChars+0x3f (0x00007FEFA6A3491F [libj9vm29.so+0x5591f])
    [javac] Java_jdk_internal_loader_NativeLibrary_findEntry0+0x2f (0x00007FEF9481CC7F [libjava.so+0xcc7f])
    [javac]  (0x00007FEF2A05EFE5 [<unknown>+0x0])
    [javac] ---------------------------------------

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hzongaro hzongaro hzongaro approved these changes

Assignees

@hzongaro hzongaro

Labels
comp:jit depends:omr Pull request is dependent on a corresponding change in OMR project:valhalla Used to track Project Valhalla related work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@a7ehuo @hzongaro