Add and use a list of methods that can skip null value store check #18464

a7ehuo · 2023-11-16T14:39:37Z

Add a list recognized methods in j9method that can skip null
check on the value stored to array elements.
ILGen: If array flattening is disabled and
skipNonNullableArrayNullStoreCheck is true, avoid generating
array element store non-helper calls.
VP: Avoid generating the nonNullableArrayNullStoreCheck if
skipNonNullableArrayNullStoreCheck is true when
transforming array element store non-helper calls.

This change depends on

Add can skip null value store check eclipse-omr/omr#7184

a7ehuo · 2023-11-23T13:39:02Z

@hzongaro Removed WIP. eclipse-omr/omr#7184 is in openj9-omr. This PR is ready for review. Thanks!

hzongaro

I still have to do a detailed review of the new list of recognized methods, but I wanted to provide some initial feedback.

Beside the minor suggestion for Value Propagation, it occurs to me that in storeArrayElement in Walker.cpp, it should be possible to avoid generating a reference to the storeFlattenableArrayElementNonHelper if non-nullable array element flattening is not enabled and IL is being generated for a safeToSkipNonNullableArrayNullStoreCheck method.

Finally, your change doesn't only add the list of methods. I would suggest that you mention in the commit and pull request description that it includes changes to Value Propagation (and maybe IL generation) to take advantage of the list of methods for which it is safe to skip the nonNullableArrayNullStoreCheck.

runtime/compiler/optimizer/J9ValuePropagation.cpp

a7ehuo · 2023-11-30T20:06:45Z

... it should be possible to avoid generating a reference to the storeFlattenableArrayElementNonHelper ..

Great point!

All above comments are addressed in 84b8f38. Ready for another review. Thank you!

hzongaro

Thanks for implementing this. I'm going to add a reference to this pull request in issue #17516, as its touches on some of the same problems with references to methods that no longer exist or methods that don't actually have array element stores that are already included in canSkipArrayStoreChecks.

runtime/compiler/il/J9MethodSymbol.cpp

- Add a list recognized methods in j9method that can skip null check on the value stored to array elements. - ILGen: If array flattening is disabled and `skipNonNullableArrayNullStoreCheck` is true, avoid generating array element store non-helper calls. - VP: Avoid generating the `nonNullableArrayNullStoreCheck` if `skipNonNullableArrayNullStoreCheck` is true when transforming array element store non-helper calls. Signed-off-by: Annabelle Huo <[email protected]>

a7ehuo · 2023-12-06T15:40:13Z

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

hzongaro · 2023-12-06T19:50:37Z

Thinking about the discussion around ComparableTimSort a bit more, it occurs to me that avoiding a run-time check to see whether the component type of the array is null-restricted is only one potential benefit that can be derived from knowing what kinds of arrays a recognized method works with. The other potential benefit would to avoid unnecessary run-time checks of whether the elements of the arrays are flattened, and so avoid generating calls to storeFlattenableArrayElementNonHelper and loadFlattenableArrayElementNonHelper altogether.

As things stand, I think ComparableTimSort and TimSort contain methods that can avoid nonNullableArrayNullStoreCheck, but would still need storeFlattenableArrayElementNonHelper and loadFlattenableArrayElementNonHelper calls. The various toArray([Object;)[Object; methods in the collections classes might end up being in a similar situation, if the RI does something to eliminate the explicit stores of a null reference. I think that for all the other methods, both nonNullableArrayNullStoreCheck and {load|store}FlattenableArrayElementNonHelper calls could be avoided.

Thoughts? If you agree, then perhaps that could be handled as a follow-on pull request.

a7ehuo · 2023-12-06T20:22:27Z

The other potential benefit would to avoid unnecessary run-time checks of whether the elements of the arrays are flattened, and so avoid generating calls to storeFlattenableArrayElementNonHelper and loadFlattenableArrayElementNonHelper altogether. ...
Thoughts? If you agree, then perhaps that could be handled as a follow-on pull request.

That's a great idea! If I understand it correctly, we could have another list of recognized methods that can skip generating the non helpers for flattenable arrays in ILGen, such as canSkipHelperCallForFlattenableArrays []. I agree it should be in a separate change/PR.

hzongaro

Thanks for the updates! Looks good.

May I ask you to open a follow-on issue as a reminder that we will have to deal with the various T[] xxx.toArray(T[]) methods?

hzongaro · 2023-12-07T15:09:28Z

Jenkins test sanity xlinuxval,xlinuxvalst jdknext

a7ehuo · 2023-12-07T15:36:47Z

May I ask you to open a follow-on issue as a reminder that we will have to deal with the various T[] xxx.toArray(T[]) methods?

#18576 is created to track this work item

hzongaro · 2023-12-07T18:54:25Z

I forgot that I shouldn't be using xlinux for PR testing.

Jenkins test sanity alinuxval,alinuxvalst jdknext

pshipton · 2023-12-07T19:47:29Z

I forgot that I shouldn't be using xlinux for PR testing.

Actually it might be working again soon. Plus a little bit of testing is fine, it's when there is too much that jenkins can restart or tests can time out. Adoptium have almost fixed the problem of downloading 100MB+ of data for every test run. There was one problem this morning with a PR open to fix it, still pending merge atm.

pshipton · 2023-12-07T19:49:13Z

Your xlinux testing did run. Starting a new PR build doesn't cancel any old builds.

hzongaro · 2023-12-07T19:59:29Z

Your xlinux testing did run. Starting a new PR build doesn't cancel any old builds.

Right, but it had failed with some unexpected errors that were likely unrelated to this PR. When I went to rerun, I realized I ought not rerun on xlinux.

hzongaro · 2023-12-08T15:23:39Z

This shouldn't affect anything with value types support disabled, but just in case:

Jenkins test sanity all jdk17

a7ehuo · 2023-12-11T15:12:54Z

openjdk17_j9_sanity.functional_x86-64_windows failed with the following error. It doesn't seem related to this PR.

12:41:56  -----------------------------------
12:41:56  cmdLineTester_AutoModuleClassloaderTest_0_PASSED
12:41:56  -----------------------------------
...
12:41:57  Attempting to destroy all caches in cacheDir C:\Users\jenkins.001\AppData\Local\javasharedresources\
12:41:57  
12:41:57  JVMSHRC241E Error: unable to delete shared class cache file
12:41:57  JVMSHRC336E Port layer error code = -100
12:41:57  JVMSHRC337E Platform error message: (32) The process cannot access the file because it is being used by another process.
12:41:57  JVMSHRC430I Failed to remove current generation of shared class cache "jenkins"
...
12:42:02  F:\Users\jenkins\workspace\Test_openjdk17_j9_sanity.functional_x86-64_windows_Personal_testList_1\aqa-tests\TKG/../TKG/settings.mk:351: \\TKG\\openj9Settings.mk: No such file or directory
12:42:02  make[6]: *** No rule to make target '\\TKG\\openj9Settings.mk'.  Stop.
...
12:42:03  Could not find test result, set build result to FAILURE.

hzongaro · 2023-12-11T22:38:46Z

Now that Pull request #18584 has been merged, Valhalla standard testing should be more successful

Jenkins test sanity alinuxvalst jdknext

hangshao0 · 2023-12-12T15:19:43Z

The "access" shared library load error in Valhalla standard build should be fixed by: ibmruntimes/openj9-openjdk-jdk.valuetypes@89c45d9. I see @JasonFengJ9 pushed the fix in 2 hours ago.

JasonFengJ9 · 2023-12-12T15:35:09Z

The "access" shared library load error in Valhalla standard build should be fixed by: ibmruntimes/openj9-openjdk-jdk.valuetypes@89c45d9. I see @JasonFengJ9 pushed the fix in 2 hours ago.

Yeah, I am running a verification build https://openj9-jenkins.osuosl.org/job/Build_JDKnext_x86-64_windows_vt_standard_Personal/5/console which is waiting for a machine.

hzongaro · 2023-12-12T20:17:35Z

Jenkins test sanity alinuxvalst jdknext

hzongaro · 2023-12-13T15:38:08Z

Failure in aarch64 Linux vt_standard testing appears to be due to issue #17844. Merging.

a7ehuo added project:valhalla Used to track Project Valhalla related work depends:omr Pull request is dependent on a corresponding change in OMR labels Nov 16, 2023

a7ehuo mentioned this pull request Nov 16, 2023

Add can skip null value store check eclipse-omr/omr#7184

Merged

1 task

a7ehuo marked this pull request as ready for review November 16, 2023 14:57

a7ehuo changed the title ~~Add a list of methods that can skip null value store check~~ WIP: Add a list of methods that can skip null value store check Nov 16, 2023

a7ehuo requested a review from hzongaro November 16, 2023 14:57

a7ehuo force-pushed the add-skip-null-value-store-check-pr branch from 79f3f82 to e4965d2 Compare November 20, 2023 18:49

a7ehuo changed the title ~~WIP: Add a list of methods that can skip null value store check~~ Add a list of methods that can skip null value store check Nov 23, 2023

hzongaro self-assigned this Nov 28, 2023

hzongaro reviewed Nov 28, 2023

View reviewed changes

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

a7ehuo force-pushed the add-skip-null-value-store-check-pr branch from e4965d2 to 84b8f38 Compare November 30, 2023 20:03

a7ehuo requested a review from dsouzai as a code owner November 30, 2023 20:03

a7ehuo changed the title ~~Add a list of methods that can skip null value store check~~ Add and use a list of methods that can skip null value store check Nov 30, 2023

hzongaro reviewed Dec 5, 2023

View reviewed changes

a7ehuo force-pushed the add-skip-null-value-store-check-pr branch from 84b8f38 to 066df6b Compare December 6, 2023 15:38

hzongaro approved these changes Dec 7, 2023

View reviewed changes

a7ehuo mentioned this pull request Dec 7, 2023

Value Types: Check the list of *.toArray() methods that can skip null check in storing into non-nullable arrays #18576

Open

hzongaro merged commit a3c7ae9 into eclipse-openj9:master Dec 13, 2023
17 of 21 checks passed

a7ehuo deleted the add-skip-null-value-store-check-pr branch March 6, 2024 16:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add and use a list of methods that can skip null value store check #18464

Add and use a list of methods that can skip null value store check #18464

a7ehuo commented Nov 16, 2023 •

edited

Loading

a7ehuo commented Nov 23, 2023

hzongaro left a comment

a7ehuo commented Nov 30, 2023

hzongaro left a comment

a7ehuo commented Dec 6, 2023 •

edited

Loading

hzongaro commented Dec 6, 2023

a7ehuo commented Dec 6, 2023

hzongaro left a comment

hzongaro commented Dec 7, 2023

a7ehuo commented Dec 7, 2023

hzongaro commented Dec 7, 2023

pshipton commented Dec 7, 2023

pshipton commented Dec 7, 2023

hzongaro commented Dec 7, 2023

hzongaro commented Dec 8, 2023

a7ehuo commented Dec 11, 2023

hzongaro commented Dec 11, 2023

hangshao0 commented Dec 12, 2023

JasonFengJ9 commented Dec 12, 2023

hzongaro commented Dec 12, 2023

hzongaro commented Dec 13, 2023

Add and use a list of methods that can skip null value store check #18464

Add and use a list of methods that can skip null value store check #18464

Conversation

a7ehuo commented Nov 16, 2023 • edited Loading

a7ehuo commented Nov 23, 2023

hzongaro left a comment

Choose a reason for hiding this comment

a7ehuo commented Nov 30, 2023

hzongaro left a comment

Choose a reason for hiding this comment

a7ehuo commented Dec 6, 2023 • edited Loading

hzongaro commented Dec 6, 2023

a7ehuo commented Dec 6, 2023

hzongaro left a comment

Choose a reason for hiding this comment

hzongaro commented Dec 7, 2023

a7ehuo commented Dec 7, 2023

hzongaro commented Dec 7, 2023

pshipton commented Dec 7, 2023

pshipton commented Dec 7, 2023

hzongaro commented Dec 7, 2023

hzongaro commented Dec 8, 2023

a7ehuo commented Dec 11, 2023

hzongaro commented Dec 11, 2023

hangshao0 commented Dec 12, 2023

JasonFengJ9 commented Dec 12, 2023

hzongaro commented Dec 12, 2023

hzongaro commented Dec 13, 2023

a7ehuo commented Nov 16, 2023 •

edited

Loading

a7ehuo commented Dec 6, 2023 •

edited

Loading