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

Set nodeCountThreshold to 16k for >= hot compilations #7337

Merged
merged 1 commit into from
May 15, 2024

Conversation

r30shah
Copy link
Contributor

@r30shah r30shah commented May 15, 2024

Set nodeCountThreshold to 16k for hot or above method compilations.

Set nodeCountThreshold to 16k for hot or above method compilations.

Signed-off-by: Rahil Shah <rahil@ca.ibm.com>
@r30shah
Copy link
Contributor Author

r30shah commented May 15, 2024

@mpirvu Can I get your review on this change ?

Copy link
Contributor

@mpirvu mpirvu left a comment

Choose a reason for hiding this comment

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

LGTM

@r30shah
Copy link
Contributor Author

r30shah commented May 15, 2024

Jenkins build all

@r30shah
Copy link
Contributor Author

r30shah commented May 15, 2024

@AdamBrousseau Seems like riscV 64 build is stuck waiting for nodes which are offline (https://ci.eclipse.org/omr/job/PullRequest-linux_riscv64_cross/1400/), is there a workaround for that?

@r30shah
Copy link
Contributor Author

r30shah commented May 15, 2024

@vijaysun-omr Seems like only remaining build is riscv-64 for which both machine where it can be run are offline which not sure will be resolved soon.
As it is the simple change, I have not ran internal testing builds and only have ran ODM-ILOG and Liberty for verification. Can launch ones internally if you want me to.

@vijaysun-omr
Copy link
Contributor

Given the comments from Rahil, and my review of this purely heuristic change to the inliner, I feel it is very likely that the testing on RISC-V would not throw up any new issues. I am merging this change on that basis rather than hold up for infra reasons.

@vijaysun-omr vijaysun-omr merged commit 0c9ff80 into eclipse:master May 15, 2024
15 of 18 checks passed
@JamesKingdon
Copy link
Contributor

JamesKingdon commented May 27, 2024

Hi @r30shah , could you give me some background on the motivation for this change? According to #7240 it looks like the larger limit would already be the default in openJ9? (for hot and above, i.e. making this a null change for openJ9)

@r30shah
Copy link
Contributor Author

r30shah commented May 27, 2024

Hi @JamesKingdon , yes I believe the main intention of the change in #7240 was to lower the count for all compiles and let the consumer like OpenJ9 to set the threshold for the compiles, but we do not do that in OpenJ9 and it is only done when an environment option TR_EnableExpensiveOptsAtWarm is set which would only set higher count for warm and below. For hot and above compilations, it should have node count threshold high enough so that the resources we put in to deciding and started compiling method at hot or above opt level can get most out of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants