Skip to content

Recognize StringCoding.implEncodeAsciiArray in the JIT compiler #18876

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
Merged
merged 1 commit into from
Feb 16, 2024
Merged

Recognize StringCoding.implEncodeAsciiArray in the JIT compiler #18876

merged 1 commit into from
Feb 16, 2024

Conversation

dchopra001
Copy link
Contributor

This commit introduces StringCoding.implEncodeAsciiArray (introduced in Java 17+) as a recognized method in the JIT compiler.

@dchopra001 dchopra001 requested a review from dsouzai as a code owner February 1, 2024 18:18
@dchopra001 dchopra001 changed the title Recognize StringCoding.implEncodeAsciiArray Recognize StringCoding.implEncodeAsciiArray in the JIT compiler Feb 1, 2024
@dchopra001 dchopra001 requested review from hzongaro and r30shah and removed request for dsouzai February 1, 2024 18:18
@dchopra001
Copy link
Contributor Author

Requesting review from @hzongaro, @r30shah

@dchopra001
Copy link
Contributor Author

Tagging @vijaysun-omr if you'd like to review as well.

@vijaysun-omr
Copy link
Contributor

I skimmed the change quickly (and it looks like the right step) but will defer to Henry or Rahil for review / merge.

hzongaro
hzongaro reviewed Feb 6, 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 in this pull request looks good. May I ask you to mention in the commit comment that there will be corresponding changes in Value Propagation in OMR that will take advantage of recognizing this method?

@hzongaro hzongaro self-assigned this Feb 6, 2024
hzongaro
hzongaro reviewed Feb 6, 2024
View reviewed changes
r30shah
r30shah reviewed Feb 6, 2024
View reviewed changes
Copy link
Contributor

@r30shah r30shah left a comment

Choose a reason for hiding this comment

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

Apart from the typo that Henry already pointed out, changes overall looks good to me. I do agree with @hzongaro . We should have bit more background in the commit message.

@dchopra001 dchopra001 force-pushed the accelImplEncodeAscii branch from 9280b69 to d34659a Compare February 6, 2024 18:57
hzongaro
hzongaro approved these changes Feb 6, 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 good. Thanks!

@hzongaro
Copy link
Member

hzongaro commented Feb 7, 2024

Sorry for the belated comment, but are there any tests that ensure StringCoding.implEncodeAsciiArray is exercised? I see it's used in the implementations of the encodeASCII methods in java/lang/System and java/lang/Access. If there are no tests, perhaps some new tests could be added to TestJavaLangStringCodingEncodeASCII.java

@dchopra001
Recognize java/lang/StringCoding.implEncodeAsciiArray
4335656 
implEncodeAsciiArray is equivalent to Java 11's
US_ASCII.encodeASCII converter method. Acceleration for
this routine already exists in OMR. This commit, along with
PR 7247 in eclipse/omr introduces changes in Value Propagation
to transform this call into an arraytranslate node and accelerate
the method.
@hzongaro
Copy link
Member

Testing together with OMR pull request 7247, which takes advantage of this change.

Jenkins test sanity all jdk17,jdk21 depends eclipse-omr/omr#7247

@hzongaro
Copy link
Member

JDK 21 Linux on z and AIX failures appear to have been due to infrastructure issues. Rerunning.

Jenkins test sanity zlinux,aix jdk21 depends eclipse-omr/omr#7247

@hzongaro
Copy link
Member

JDK 21 AIX build failed with this error this time:

10:47:00  === Output from failing command(s) repeated here ===
10:47:00  * For target jdk_modules_java.scripting__the.java.scripting_batch:
10:47:00  IOException caught during compilation: Deadlock condition if locked

That appears to be known issue #8625. Trying once more.

Jenkins test sanity aix jdk21 depends eclipse-omr/omr#7247

@hzongaro
Copy link
Member

Testing with upstream OMR change that takes advantage of this change was successful. Merging.

@hzongaro hzongaro merged commit 6a6d7af into eclipse-openj9:master Feb 16, 2024
@pshipton
Copy link
Member

I suspect this change, by itself since OMR hasn't promoted, has caused a lot of crashes in the builds.
#18979

@hzongaro
Copy link
Member

I suspect this change, by itself since OMR hasn't promoted, has caused a lot of crashes in the builds.

I had thought this change would have no effect without the related OMR change. I'll revert both.

@pshipton
Copy link
Member

I'm not sure the cause is this change. Also I've just promoted eclipse-omr/omr#7247 since the OMR acceptance build looked ok.

@hzongaro hzongaro mentioned this pull request Mar 28, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@r30shah r30shah r30shah left review comments

@hzongaro hzongaro hzongaro approved these changes

Assignees

@hzongaro hzongaro

Labels
comp:jit perf
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dchopra001 @vijaysun-omr @hzongaro @pshipton @r30shah