Value type null-restricted array support #19911

theresa-m · 2024-07-23T20:19:30Z

Please note this pull request adds support to create a null-restricted array but not a flattened array. That will come in a later change.

create a null-restricted array field for every RAM class if one doesn't exist when a null-restricted array is created (see Add implementation for new JNI functions in Valhalla #19460 (comment))
implement JVM_IsNullRestrictedArray and JVM_NewNullRestrictedArray
Update exception thrown when null is set to a null-restricted array from NullPointerException to ArrayStoreException as described in https://openjdk.org/jeps/8316779

Related: #17340
Fixes: #19460

After these changes:

ValueTypeArrayTests have been enabled and pass Add implementation for new JNI functions in Valhalla #19460 (comment)
ValueTypeTestsJIT still has some failures Fix tests in disabled variations of ValueTypeTestsJIT #19708
ValueTypeArrayTestsJIT still has some failures Fix ValueTypeArrayTestsJIT test failures #19913
ValueTypeSystemArraycopyTests pass for Xint variations Fix tests in disabled variations of ValueTypeSystemArraycopyTests #19914

runtime/j9vm/javanextvmi.cpp

runtime/oti/j9nonbuilder.h

runtime/vm/createramclass.cpp

hangshao0 · 2024-07-24T18:18:50Z

Before continue reviewing this, I am wondering if it is easier to add an new function
internalCreateArrayClassWithOptions(J9VMThread* vmThread, J9ROMArrayClass* romClass, J9Class* elementClass, UDATA options) (The existing internalCreateArrayClass() becomes a special case that passed in options is 0) and call this function in JVM_NewNullRestrictedArray with a new option flag J9_FINDCLASS_FLAG_CLASS_OPTION_NULL_RESTRICTED_ARRAY indicating it is creating a null restricted array.

runtime/j9vm/javanextvmi.cpp

runtime/vm/createramclass.cpp

runtime/oti/j9nonbuilder.h

hangshao0 · 2024-07-24T19:39:15Z

Before continue reviewing this, I am wondering if it is easier to add an new function
internalCreateArrayClassWithOptions(J9VMThread* vmThread, J9ROMArrayClass* romClass, J9Class* elementClass, UDATA options) (The existing internalCreateArrayClass() becomes a special case that passed in options is 0) and call this function in JVM_NewNullRestrictedArray with a new option flag J9_FINDCLASS_FLAG_CLASS_OPTION_NULL_RESTRICTED_ARRAY indicating it is creating a null restricted array.

The reason for this is to see if we can minimize the change in createramclass.cpp and do not need special handling of pushing/poping the protectionDomain and class loading stack. The nullable J9arrayClass and null restricted J9arrayClass can be created separately, one in each internalCreateArrayClass*() call.

theresa-m · 2024-07-30T15:30:07Z

Before continue reviewing this, I am wondering if it is easier to add an new function
internalCreateArrayClassWithOptions(J9VMThread* vmThread, J9ROMArrayClass* romClass, J9Class* elementClass, UDATA options) (The existing internalCreateArrayClass() becomes a special case that passed in options is 0) and call this function in JVM_NewNullRestrictedArray with a new option flag J9_FINDCLASS_FLAG_CLASS_OPTION_NULL_RESTRICTED_ARRAY indicating it is creating a null restricted array.

The reason for this is to see if we can minimize the change in createramclass.cpp and do not need special handling of pushing/poping the protectionDomain and class loading stack. The nullable J9arrayClass and null restricted J9arrayClass can be created separately, one in each internalCreateArrayClass*() call.

I agree this approach may be cleaner, I'll give it a try and look over the review comments that are still applicable.

ThanHenderson

Just a few formatting comments.

runtime/j9vm/javanextvmi.cpp

runtime/vm/createramclass.cpp

hangshao0 · 2024-08-01T18:24:41Z

The following GC code need to be updated, so that GC scan is able to find the new null restricted array class:

openj9/runtime/gc_structs/ClassArrayClassSlotIterator.cpp

Line 40 in 5b6168a

GC_ClassArrayClassSlotIterator::nextSlot()

openj9/runtime/gc_structs/ClassLoaderClassesIterator.cpp

Line 103 in 5b6168a

GC_ClassLoaderClassesIterator::nextClass()

We provide helpers to JIT in cnathelper.cpp. I see functions there are still using J9_IS_J9CLASS_PRIMITIVE_VALUETYPE and throw NPE if null is stored to null restricted array. They need to be updated to check the null restricted array and throw ASE.
cnathelper.cpp can be changed in another PR, and you may not be able to update some of them without JIT changing there code to pass you the nullRestrictedArrayClass.

theresa-m · 2024-08-01T19:16:22Z

~~The JIT updates should be covered by #19913.~~
Discussed in slack, I will update cnathelper.cpp and check if the referenced issue can be closed.

I'll take a look at the GC code.

runtime/codert_vm/cnathelp.cpp

runtime/gc_structs/ClassArrayClassSlotIterator.cpp

runtime/gc_structs/ClassLoaderClassesIterator.cpp

hangshao0 · 2024-08-02T19:38:59Z

@dmitripivkine I added you to review the GC change here.

The change here is to support value type which could have 2 types of arrays. One allows its element to be null, one does not allow its element to be null (null restricted array). A new field nullRestrictedArrayClass is added in J9Class. (Note nullRestrictedArrayClass can only be 1D array).

runtime/gc_structs/ClassArrayClassSlotIterator.cpp

keithc-ca

In the files changed by this pull request, there are still several occurrences of "null restricted" that should be "null-restricted".

Please also address the line-end check failure.

theresa-m · 2024-08-29T14:09:16Z

In the files changed by this pull request, there are still several occurrences of "null restricted" that should be "null-restricted".

I think they have all been fixed in the code and commit messages. Which others are you seeing?

- create a null-restricted array field for every RAM class if one doesn't exist when a null-restricted array is created - implement JVM_IsNullRestrictedArray and JVM_NewNullRestrictedArray - Update exception thrown when null is set to a null-restricted array from NullPointerException to ArrayStoreException as described in https://openjdk.org/jeps/8316779 Signed-off-by: Theresa Mammarella <[email protected]>

keithc-ca · 2024-08-29T16:03:32Z

Which others are you seeing

vm_api.h - lines 2715, 2719, 2823, etc.
BytecodeInterpreter.hpp - line 8585 and missing comma on line 8584
ValueTypeHelpers.hpp - lines 193, 197
ValueTypeTest.java - lines 196, 1804, 2262

theresa-m · 2024-08-29T16:10:13Z

Which others are you seeing

vm_api.h - lines 2715, 2719, 2823, etc.

BytecodeInterpreter.hpp - line 8585 and missing comma on line 8584

ValueTypeHelpers.hpp - lines 193, 197

ValueTypeTest.java - lines 196, 1804, 2262

Do you mind if I do this as a separate pull requests? This is out of scope of the current change since none of those code sections are being modified.

keithc-ca · 2024-08-29T16:28:42Z

do this as a separate pull request

That is fine with me, but, I'd like to see those changes ready to go, perhaps even before this is merged (so it does not get forgotten).

theresa-m · 2024-08-29T17:17:50Z

Okay. I have fixed the remaining cases in #20085

keithc-ca · 2024-08-29T19:53:41Z

Jenkins test sanity aarch64_linux_vt_standard,s390x_linux

keithc-ca · 2024-08-29T20:02:09Z

Jenkins test sanity aarch64_linux_vt_standard,s390x_linux jdknext

hangshao0 · 2024-08-29T20:48:44Z

Jenkins test sanity,extended zlinuxval jdknext

theresa-m · 2024-08-30T13:00:16Z

I'm looking at the test failures.

The purpose of this change is to keep value type command line tests passing until they are updated and the vm fully enables flattening of null-restricted arrays. Signed-off-by: Theresa Mammarella <[email protected]>

theresa-m · 2024-08-30T20:58:33Z

For the sanity failures many of them seem to already exist. I ran sanity tests for a value types build on the master branch and similar failures occurred. https://hyc-runtimes-jenkins.swg-devops.com/view/OpenJ9%20-%20Personal/job/Pipeline-Build-Test-Personal/23714/
I'm not sure where that started since the valhalla nightly builds don't run full sanity tests, I will need to do more testing.
I pushed a second commit to fix the extended failures. The tests are currently written to use arrayClass so to keep them passing both arrayClass and nullRestrictedArrayClass can have the J9ClassIsFlattened set for now. The purpose of this pull request was to add support for null-restricted arrays without flattening support. I have a draft pull request Null-restricted array flattening and test fix up #19995 that will address flattening where I update the value type command line tests and will stop setting J9ClassIsFlattened for arrayClass.

failing pr build links so they aren't lost:
https://openj9-jenkins.osuosl.org/job/PullRequest-OpenJ9/5985/
https://openj9-jenkins.osuosl.org/job/Test_openjdknext_j9_sanity.functional_aarch64_linux_vt_standard_Personal/8/
https://openj9-jenkins.osuosl.org/job/Test_openjdknext_j9_sanity.functional_s390x_linux_Personal/62/
https://openj9-jenkins.osuosl.org/job/Test_openjdkValhalla_j9_extended.functional_s390x_linux_valhalla_Personal/27/
https://openj9-jenkins.osuosl.org/job/Test_openjdkValhalla_j9_sanity.functional_s390x_linux_valhalla_Personal/29/

hangshao0 · 2024-09-03T13:40:34Z

Jenkins test sanity,extended zlinuxval jdknext

hangshao0 · 2024-09-03T13:52:06Z

Jenkins compile amac jdknext

Signed-off-by: Theresa Mammarella <[email protected]>

theresa-m · 2024-09-03T17:05:40Z

The failures in ValueTypeTestsJIT are related to 42393ff and array flattening. I have disabled them for now since there is more work to be done to flatten arrays in #19995

hangshao0 · 2024-09-03T17:56:29Z

Jenkins test sanity,extended zlinuxval jdknext

pshipton · 2024-09-04T01:15:18Z

I believe this is causing hangs in testing. I'll revert it while we investigate.

pshipton · 2024-09-04T01:15:57Z

Reverted via #20109

theresa-m added comp:vm project:valhalla Used to track Project Valhalla related work labels Jul 23, 2024

theresa-m force-pushed the nr_array branch 2 times, most recently from 9c741db to bf4b5b3 Compare July 23, 2024 20:38

This was referenced Jul 24, 2024

Fix ValueTypeArrayTestsJIT test failures #19913

Open

Fix tests in disabled variations of ValueTypeSystemArraycopyTests #19914

Closed

theresa-m force-pushed the nr_array branch from bf4b5b3 to f9a202d Compare July 24, 2024 13:30

theresa-m marked this pull request as ready for review July 24, 2024 13:35

theresa-m requested a review from hangshao0 July 24, 2024 13:35

hangshao0 reviewed Jul 24, 2024

View reviewed changes

runtime/j9vm/javanextvmi.cpp Outdated Show resolved Hide resolved

runtime/oti/j9nonbuilder.h Outdated Show resolved Hide resolved

runtime/vm/createramclass.cpp Outdated Show resolved Hide resolved

ThanHenderson reviewed Jul 24, 2024

View reviewed changes

runtime/j9vm/javanextvmi.cpp Outdated Show resolved Hide resolved

ThanHenderson reviewed Jul 24, 2024

View reviewed changes

theresa-m force-pushed the nr_array branch from f9a202d to 7893cc6 Compare July 31, 2024 15:05

theresa-m requested review from hangshao0 and ThanHenderson July 31, 2024 15:06

theresa-m force-pushed the nr_array branch from 7893cc6 to 8a2cc2e Compare July 31, 2024 15:24

ThanHenderson reviewed Jul 31, 2024

View reviewed changes

runtime/j9vm/javanextvmi.cpp Outdated Show resolved Hide resolved

runtime/j9vm/javanextvmi.cpp Outdated Show resolved Hide resolved

runtime/vm/createramclass.cpp Show resolved Hide resolved

theresa-m force-pushed the nr_array branch from 8a2cc2e to 33f5c3c Compare July 31, 2024 17:01

hangshao0 reviewed Aug 1, 2024

View reviewed changes

runtime/vm/createramclass.cpp Outdated Show resolved Hide resolved

theresa-m force-pushed the nr_array branch from 33f5c3c to 340d9c0 Compare August 2, 2024 17:33

theresa-m requested a review from hangshao0 August 2, 2024 17:34

hangshao0 reviewed Aug 2, 2024

View reviewed changes

hangshao0 requested a review from dmitripivkine August 2, 2024 19:32

dmitripivkine reviewed Aug 6, 2024

View reviewed changes

runtime/gc_structs/ClassArrayClassSlotIterator.cpp Outdated Show resolved Hide resolved

keithc-ca requested changes Aug 29, 2024

View reviewed changes

theresa-m force-pushed the nr_array branch from bc6b453 to 1a2c4bc Compare August 29, 2024 14:09

theresa-m mentioned this pull request Aug 29, 2024

Correct "null restricted" to null-restricted #20085

Merged

theresa-m mentioned this pull request Aug 29, 2024

J9ClassHasIdentity should not be set for interfaces #20086

Merged

tajila approved these changes Aug 30, 2024

View reviewed changes

Flatten nullable and null-restricted arrays

42393ff

The purpose of this change is to keep value type command line tests passing until they are updated and the vm fully enables flattening of null-restricted arrays. Signed-off-by: Theresa Mammarella <[email protected]>

Disable failing flattened array tests

b3efa28

Signed-off-by: Theresa Mammarella <[email protected]>

hangshao0 merged commit c28eae6 into eclipse-openj9:master Sep 3, 2024
6 checks passed

pshipton mentioned this pull request Sep 4, 2024

Revert "Value type null-restricted array support" #20109

Merged

This was referenced Sep 4, 2024

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

Merged

Fix null restricted array related issues for value types #20112

Merged

This was referenced Sep 9, 2024

Value type null-restricted array support #20132

Merged

Move value type jni methods to their own file #20138

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Value type null-restricted array support #19911

Value type null-restricted array support #19911

theresa-m commented Jul 23, 2024 •

edited

Loading

hangshao0 commented Jul 24, 2024

hangshao0 commented Jul 24, 2024 •

edited

Loading

theresa-m commented Jul 30, 2024

ThanHenderson left a comment

hangshao0 commented Aug 1, 2024

theresa-m commented Aug 1, 2024 •

edited

Loading

hangshao0 commented Aug 2, 2024

keithc-ca left a comment

theresa-m commented Aug 29, 2024

keithc-ca commented Aug 29, 2024

theresa-m commented Aug 29, 2024

keithc-ca commented Aug 29, 2024

theresa-m commented Aug 29, 2024

keithc-ca commented Aug 29, 2024

keithc-ca commented Aug 29, 2024

hangshao0 commented Aug 29, 2024

theresa-m commented Aug 30, 2024

theresa-m commented Aug 30, 2024 •

edited

Loading

hangshao0 commented Sep 3, 2024

hangshao0 commented Sep 3, 2024

theresa-m commented Sep 3, 2024

hangshao0 commented Sep 3, 2024

pshipton commented Sep 4, 2024

pshipton commented Sep 4, 2024

Value type null-restricted array support #19911

Value type null-restricted array support #19911

Conversation

theresa-m commented Jul 23, 2024 • edited Loading

hangshao0 commented Jul 24, 2024

hangshao0 commented Jul 24, 2024 • edited Loading

theresa-m commented Jul 30, 2024

ThanHenderson left a comment

Choose a reason for hiding this comment

hangshao0 commented Aug 1, 2024

theresa-m commented Aug 1, 2024 • edited Loading

hangshao0 commented Aug 2, 2024

keithc-ca left a comment

Choose a reason for hiding this comment

theresa-m commented Aug 29, 2024

keithc-ca commented Aug 29, 2024

theresa-m commented Aug 29, 2024

keithc-ca commented Aug 29, 2024

theresa-m commented Aug 29, 2024

keithc-ca commented Aug 29, 2024

keithc-ca commented Aug 29, 2024

hangshao0 commented Aug 29, 2024

theresa-m commented Aug 30, 2024

theresa-m commented Aug 30, 2024 • edited Loading

hangshao0 commented Sep 3, 2024

hangshao0 commented Sep 3, 2024

theresa-m commented Sep 3, 2024

hangshao0 commented Sep 3, 2024

pshipton commented Sep 4, 2024

pshipton commented Sep 4, 2024

theresa-m commented Jul 23, 2024 •

edited

Loading

hangshao0 commented Jul 24, 2024 •

edited

Loading

theresa-m commented Aug 1, 2024 •

edited

Loading

theresa-m commented Aug 30, 2024 •

edited

Loading