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.

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

Fix missing diagnostics and tuning knobs in inliner #20890

Merged
merged 1 commit into from
Jan 15, 2025

Conversation

vijaysun-omr
Copy link
Contributor

@vijaysun-omr vijaysun-omr commented Jan 7, 2025

Several inlining parameters can be tuned via command line options or env vars. Recently, I needed to tune some inlining parameters that could not be controlled without changing the source. This PR fixes the cases that I observed by adding env vars. I also added some heuristic trace diagnostics that print when we estimate a call but don't inline it.

@vijaysun-omr vijaysun-omr changed the title WIP : Adding an env var for inliner max recursion depth WIP : Fix missing diagnostics and tuning knobs in inliner Jan 7, 2025
@vijaysun-omr vijaysun-omr force-pushed the inliner-diags2 branch 2 times, most recently from 38f3428 to b7f9403 Compare January 8, 2025 13:50
@vijaysun-omr vijaysun-omr changed the title WIP : Fix missing diagnostics and tuning knobs in inliner Fix missing diagnostics and tuning knobs in inliner Jan 8, 2025
@vijaysun-omr
Copy link
Contributor Author

@hzongaro and @gita-omr this may be of interest to you for review

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. Just a couple of minor suggestions.

runtime/compiler/optimizer/J9EstimateCodeSize.cpp Outdated Show resolved Hide resolved
runtime/compiler/optimizer/J9EstimateCodeSize.cpp Outdated Show resolved Hide resolved
runtime/compiler/optimizer/J9EstimateCodeSize.cpp Outdated Show resolved Hide resolved
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!

@hzongaro hzongaro self-assigned this Jan 8, 2025
@gita-omr
Copy link
Contributor

gita-omr commented Jan 8, 2025

LGTM

@hzongaro
Copy link
Member

hzongaro commented Jan 8, 2025

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

@hzongaro
Copy link
Member

hzongaro commented Jan 9, 2025

JDK 8 sanity.functional failure appears to be infrastructure-related. Rerunning. . . .

Jenkins test sanity.functional xlinux jdk8

@hzongaro
Copy link
Member

hzongaro commented Jan 9, 2025

Jenkins test sanity.functional xlinux jdk8

@hzongaro
Copy link
Member

@vijaysun-omr, may I ask you to take a quick look at the failures that occurred in the JDK 23 sanity.openjdk testing? I think it's extremely unlikely that they are related to your changes, but I couldn't find previous examples of those failures.

@a7ehuo
Copy link
Contributor

a7ehuo commented Jan 13, 2025

I took a quick look at the recent JDK23 sanity.openjdk nightly build tests. I don't see the failure from the JDK 23 sanity.openjdk in the nightly build. I'll see if the failure is reproducible without this PR, although I also think it is very unlikely that this change could have caused the failure.

[2025-01-08T23:52:46.840Z] FAILED test targets:
[2025-01-08T23:52:46.840Z] 	jdk_util_0 - Test results: passed: 972; failed: 2; error: 1 
[2025-01-08T23:52:46.840Z] 		Failed test cases: 
[2025-01-08T23:52:46.840Z] 			TEST: java/util/TimeZone/SimpleTimeZoneCloneRaceTest.java
[2025-01-08T23:52:46.840Z]         TEST: java/util/TimeZone/TimeZoneRegression.java
[2025-01-08T23:52:46.840Z]         TEST: java/util/TimeZone/TimeZoneTest.java
[2025-01-08T23:29:50.151Z] STARTED    TimeZoneRegression::Test4109314 'Test4109314()'
[2025-01-08T23:29:50.151Z] java.lang.NullPointerException: Cannot invoke "java.util.TimeZone.getOffset(long)" because "tz" is null
[2025-01-08T23:29:50.151Z] 	at java.base/java.util.GregorianCalendar.computeFields(GregorianCalendar.java:2323)
...
[2025-01-08T23:30:01.835Z] STARTED    TimeZoneTest::TestRuleAPI 'TestRuleAPI()'
[2025-01-08T23:30:01.835Z] java.lang.IllegalArgumentException: Illegal end month 7447504
[2025-01-08T23:30:01.835Z] 	at java.base/java.util.SimpleTimeZone.decodeEndRule(SimpleTimeZone.java:1425)
...
[2025-01-08T23:30:01.838Z] ----------- Stack Backtrace -----------
[2025-01-08T23:30:01.838Z] getOriginalROMMethodUnchecked+0x23 (0x00007F88C175DE43 [libjclse29.so+0x6be43])
[2025-01-08T23:30:01.838Z] getOriginalROMMethod+0x3d (0x00007F88C175DF6D [libjclse29.so+0x6bf6d])
[2025-01-08T23:30:01.838Z] getMethodParametersAsArray+0x1a3 (0x00007F88C1729EF3 [libjclse29.so+0x37ef3])
[2025-01-08T23:30:01.838Z]  (0x00007F884657FEA8 [<unknown>+0x0])

@a7ehuo
Copy link
Contributor

a7ehuo commented Jan 14, 2025

The TimeZone test failures are not reproducible with either the JDK23 nightly build or the build with this PR change

  • 108x runs of jdk_util_0 with JDK23 nightly build: Grinder 46433, 46446, 46449
  • 108x runs of jdk_util_0 with JDK23 build with this PR change: Grinder 46447, 46450

@hzongaro
Copy link
Member

The TimeZone test failures are not reproducible with either the JDK23 nightly build or the build with this PR change

@a7ehuo, thanks for looking into that! As this seems to be a very safe change, all other testing was successful, and it seems very unlikely that the failure was related to this change, I will go ahead and merge this pull request.

@hzongaro hzongaro merged commit 006ef73 into eclipse-openj9:master Jan 15, 2025
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants