Skip to content

Recognize StringCoding.implEncodeAsciiArray in the JIT compiler - #18876

Merged
hzongaro merged 1 commit into
eclipse-openj9:masterfrom
dchopra001:accelImplEncodeAscii
Feb 16, 2024
Merged

Recognize StringCoding.implEncodeAsciiArray in the JIT compiler#18876
hzongaro merged 1 commit into
eclipse-openj9:masterfrom
dchopra001:accelImplEncodeAscii

Conversation

@dchopra001

@dchopra001 dchopra001 commented Feb 1, 2024

Copy link
Copy Markdown
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

dchopra001 commented Feb 1, 2024

Copy link
Copy Markdown
Contributor Author

Requesting review from @hzongaro, @r30shah

@dchopra001

dchopra001 commented Feb 1, 2024

Copy link
Copy Markdown
Contributor Author

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

@vijaysun-omr

vijaysun-omr commented Feb 1, 2024

Copy link
Copy Markdown
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

@hzongaro hzongaro left a comment

Copy link
Copy Markdown
Member

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
Comment thread runtime/compiler/env/j9method.cpp Outdated
r30shah
r30shah reviewed Feb 6, 2024
View reviewed changes

@r30shah r30shah left a comment

Copy link
Copy Markdown
Contributor

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

@hzongaro hzongaro left a comment

Copy link
Copy Markdown
Member

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

hzongaro commented Feb 7, 2024

Copy link
Copy Markdown
Member

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

hzongaro commented Feb 15, 2024

Copy link
Copy Markdown
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

hzongaro commented Feb 16, 2024

Copy link
Copy Markdown
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

hzongaro commented Feb 16, 2024

Copy link
Copy Markdown
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

hzongaro commented Feb 16, 2024

Copy link
Copy Markdown
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

pshipton commented Feb 20, 2024

Copy link
Copy Markdown
Member

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

@hzongaro

hzongaro commented Feb 20, 2024

Copy link
Copy Markdown
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

pshipton commented Feb 20, 2024

Copy link
Copy Markdown
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