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

JProfiling fix to avoid too many internal pointers #21056

Merged
merged 1 commit into from
Feb 5, 2025

Conversation

vijaysun-omr
Copy link
Contributor

Jprofiling adds control flow to the IL as part of the lowering of the calls to profile values. This control flow splits blocks, and in the process creates temps for values commoned across the split point. If there are enough internal pointers commoned in the method, the lower may end up creating more than the max number of internal pointers allowed, thus causing the compile to fail. This commit keeps track of how many internal pointers are created and avoids lowering any more calls once we have reached a certain threshold. Also added an env var to control this threshold for experimenting in the future.

@vijaysun-omr
Copy link
Contributor Author

@r30shah your review would be appreciated

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.

Changes looks good to me.

@r30shah
Copy link
Contributor

r30shah commented Feb 3, 2025

Jenkins test sanity.functional,sanity.openjdk xlinux,plinux,zlinux,alinux jdk21

@vijaysun-omr vijaysun-omr changed the title WIP : JProfiling fix to avoid too many internal pointers JProfiling fix to avoid too many internal pointers Feb 3, 2025
@JamesKingdon
Copy link
Contributor

JamesKingdon commented Feb 3, 2025

i ran this change with a testcase that was hitting the internal pointer limit and found that it resolved the problem.

@vijaysun-omr vijaysun-omr changed the title JProfiling fix to avoid too many internal pointers WIP : JProfiling fix to avoid too many internal pointers Feb 4, 2025
Jprofiling adds control flow to the IL as part of the lowering of
the calls to profile values. This control flow splits blocks, and
in the process creates temps for values commoned across the split
point. If there are enough internal pointers commoned in the method,
the lower may end up creating more than the max number of internal
pointers allowed, thus causing the compile to fail. This commit
keeps track of how many internal pointers are created and avoids
lowering any more calls once we have reached a certain threshold.
Also added an env var to control this threshold for experimenting
in the future.

Signed-off-by: Vijay Sundaresan <[email protected]>
@vijaysun-omr vijaysun-omr changed the title WIP : JProfiling fix to avoid too many internal pointers JProfiling fix to avoid too many internal pointers Feb 4, 2025
@r30shah
Copy link
Contributor

r30shah commented Feb 4, 2025

Here is the link of the PR build : https://openj9-jenkins.osuosl.org/job/PullRequest-OpenJ9/6901/

Looking at the failures,
S390x failures,

  1. J9vmTest_4 : Same issue as J9vmTest_0 FindDeadlockTest Testing chained deadlocks Wrong deadlocked threads #16493
  2. cmdLineTester_getPid_0 : cmdLineTester_getPid_0 RuntimeMXBean.getPID() returned 0 instead of #13085
  3. testSCCMLTests1_openj9_1 : testSCCMLTests1_openj9 Add ROM classes to the existing shared cache with line numbers by using mprotect=all abort setExtraStartupHints #20953
  4. testJitserverArguments_0 : testJitserverArguments, Test SSL success condition - failed to configure SSL ecdh #18599

Apart from those, I see cmdLineTester_callsitedbgddrext_openj9_0 and cmdLineTester_GCRegressionTests_1 failing with unable to find relevant existing issue and testJITServer_1 failed on both Linux on x86 for which I couldn't find matching existing issue (Though, a 5x grinder on x86 passes : https://openj9-jenkins.osuosl.org/job/Grinder/4085/

As change is making JProfilingValue optimization more conservative in bailing out the lowering the JProfilingValue calls when it hits the limit, it is highly unlikely to cause the above issue. For sanity I am going to launch another grinder with sanity.functional on x86 and LoZ before merging.

@r30shah
Copy link
Contributor

r30shah commented Feb 4, 2025

Jenkins test sanity.functional xlinux,zlinux jdk21

@r30shah
Copy link
Contributor

r30shah commented Feb 4, 2025

Failing test on s390b and x86 [1] linux are related to JIT server and seen on other builds as well : #21066.

Relaunching the test to get clean one

[1]. https://openj9-jenkins.osuosl.org/job/Test_openjdk21_j9_sanity.functional_x86-64_linux_Personal_testList_1/241/console

@r30shah
Copy link
Contributor

r30shah commented Feb 5, 2025

Jenkins test sanity.functional xlinux jdk21

@r30shah
Copy link
Contributor

r30shah commented Feb 5, 2025

sanity build for xlinux passes and zlinux failure was related to #21066 and other known issues which is not caused by changes in this PR. Merging the change based on amount of testing done.

@r30shah r30shah merged commit 8bbc086 into eclipse-openj9:master Feb 5, 2025
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants