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

Disclaim Cold Code Cache #19397

Merged
merged 1 commit into from
May 30, 2024
Merged

Conversation

gita-omr
Copy link
Contributor

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

  • 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

Fixed line endings

@gita-omr
Copy link
Contributor Author

Fixed EOF.

@mpirvu mpirvu self-assigned this Apr 26, 2024
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
@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/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.

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/runtime/J9CodeCache.cpp Outdated Show resolved Hide resolved
@gita-omr
Copy link
Contributor Author

Addressed latest comments.

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.

}
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?

- 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.

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

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
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.

2 participants