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.

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

Disclaim Cold Code Cache #19397

Merged
merged 1 commit into from
May 30, 2024
Merged

Disclaim Cold Code Cache #19397

merged 1 commit into from
May 30, 2024

Conversation

gita-omr
Copy link
Contributor

@gita-omr gita-omr commented Apr 26, 2024 •
edited

  • use the same heuristcs for code cache disclaim as for data cache
  • disclaim starting from the cold code
  • move stack overflow outline instructions into the warm area to increase disclaim efficiency
  • code is enabled with -Xjit:enableCodeCacheDisclaiming

@gita-omr gita-omr requested a review from dsouzai as a code owner April 26, 2024 21:11
@gita-omr gita-omr requested a review from mpirvu April 26, 2024 21:13
@gita-omr
Copy link
Contributor Author

Fixed line endings

@gita-omr
Copy link
Contributor Author

Fixed EOF.

@mpirvu mpirvu self-assigned this Apr 26, 2024
mpirvu
mpirvu reviewed Apr 30, 2024
View reviewed changes
runtime/compiler/control/HookedByTheJit.cpp Show resolved Hide resolved
runtime/compiler/control/rossa.cpp Outdated Show resolved Hide resolved
runtime/compiler/runtime/J9CodeCache.cpp Show resolved Hide resolved
runtime/compiler/runtime/J9CodeCache.cpp Show resolved Hide resolved
runtime/compiler/runtime/J9CodeCache.cpp Outdated Show resolved Hide resolved
runtime/compiler/runtime/J9CodeCacheManager.cpp Outdated Show resolved Hide resolved
runtime/compiler/runtime/J9CodeCache.cpp Outdated Show resolved Hide resolved
runtime/compiler/runtime/J9CodeCacheManager.cpp Show resolved Hide resolved
@gita-omr
Copy link
Contributor Author

gita-omr commented May 1, 2024

Addressed comments above.

@gita-omr
Copy link
Contributor Author

gita-omr commented May 1, 2024

Fixed line ending.

@gita-omr
Copy link
Contributor Author

gita-omr commented May 5, 2024

Removed dependence on eclipse/omr#7300 since it's been merged.

@gita-omr
Copy link
Contributor Author

gita-omr commented May 6, 2024

Added commit to put thunks into warm code cache.

@gita-omr
Copy link
Contributor Author

Line ending check is not related to my change - many test files are reported.

mpirvu
mpirvu reviewed May 23, 2024
View reviewed changes
runtime/compiler/control/HookedByTheJit.cpp Outdated Show resolved Hide resolved
runtime/compiler/control/HookedByTheJit.cpp Outdated Show resolved Hide resolved
runtime/compiler/control/HookedByTheJit.cpp Outdated Show resolved Hide resolved
runtime/compiler/control/J9Options.cpp Show resolved Hide resolved
runtime/compiler/control/OptionsPostRestore.cpp Show resolved Hide resolved
runtime/compiler/control/OptionsPostRestore.cpp Show resolved Hide resolved
runtime/compiler/runtime/J9CodeCache.cpp Outdated Show resolved Hide resolved
@gita-omr
Copy link
Contributor Author

Addressed latest comments.

mpirvu
mpirvu approved these changes May 28, 2024
View reviewed changes
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. I only have a small suggestion for more clarity in one of the vlog messages.

runtime/compiler/runtime/J9CodeCache.cpp Outdated
}
else if (TR::Options::getCmdLineOptions()->getVerboseOption(TR_VerbosePerformance))
{
TR_VerboseLog::writeLineLocked(TR_Vlog_INFO, "In code cache %p small pages start from %p\n", this, middle);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer this message to be more explicit. I am trying to imagine what a casual consumer of verbose logs would understand when it sees this message among all the other others.
Suggestion: "Forcing code cache cold region %p-%p to use default size memory pages"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@gita-omr
Copy link
Contributor Author

Addressed latest comment.

@gita-omr
Copy link
Contributor Author

Squashed the commits.

@gita-omr
Copy link
Contributor Author

Trailing spaces are not due to this PR.

@mpirvu
Copy link
Contributor

mpirvu commented May 28, 2024

Could you please rebase to eliminate conflicts?

@gita-omr
Disclaim Cold Code Cache
c3c689a 
- use the same heuristcs for code cache disclaim as for data cache
- disclaim starting from the cold code
- move stack overflow outline instructions into the warm area to
  increase disclaim efficiency
- code is enabled with -Xjit:enableCodeCacheDisclaiming
@gita-omr
Copy link
Contributor Author

Rebased.

mpirvu
mpirvu approved these changes May 29, 2024
View reviewed changes
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.
I have a question though: have you tested what happens when large pages are enabled on the system?
I have seen that VM code uses shmat instead of mmap when large pages are configured on the system. I would assume that disclaiming is not going to work in that case. Do we take steps to disable it?

@mpirvu
Copy link
Contributor

mpirvu commented May 29, 2024

jenkins test sanity all jdk17

@gita-omr
Copy link
Contributor Author

gita-omr commented May 29, 2024 •
edited

LGTM. I have a question though: have you tested what happens when large pages are enabled on the system? I have seen that VM code uses shmat instead of mmap when large pages are configured on the system. I would assume that disclaiming is not going to work in that case. Do we take steps to disable it?

Good question. I did not try it on the system with large pages. I will take a note and open another PR. This PR is just the core code. I was planning to do more tuning and enable it by default eventually.

@mpirvu
Copy link
Contributor

mpirvu commented May 30, 2024

jenkins compile win jdk17

@mpirvu mpirvu merged commit b656fc6 into eclipse-openj9:master May 30, 2024
17 checks passed
@pshipton pshipton mentioned this pull request May 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpirvu mpirvu mpirvu approved these changes

@dsouzai dsouzai Awaiting requested review from dsouzai dsouzai is a code owner

Assignees

@mpirvu mpirvu

Labels
comp:jit
Projects
None yet
Milestone
Release 0.46 (Java 8, 11, 17, 21, 22)...
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@gita-omr @mpirvu