Skip to content

Fix code cache segregation logic - #7960

Merged
dsouzai merged 1 commit into
eclipse-omr:masterfrom
KavinSatheeskumar:code_cache_segregation_fixes
Oct 6, 2025
Merged

Fix code cache segregation logic#7960
dsouzai merged 1 commit into
eclipse-omr:masterfrom
KavinSatheeskumar:code_cache_segregation_fixes

Conversation

@KavinSatheeskumar

@KavinSatheeskumar KavinSatheeskumar commented Sep 29, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor
  • correct code allocation code to respect code cache kind when it can

@KavinSatheeskumar
KavinSatheeskumar force-pushed the code_cache_segregation_fixes branch from 835ace3 to a90f2f6 Compare September 29, 2025 20:17
@KavinSatheeskumar KavinSatheeskumar mentioned this pull request Sep 30, 2025
Merged
@0xdaryl

0xdaryl commented Oct 1, 2025

Copy link
Copy Markdown
Contributor

@mpirvu : please review

@0xdaryl

0xdaryl commented Oct 1, 2025

Copy link
Copy Markdown
Contributor

I didn't review the contents of the PR (yet), but I have administrative feedback. Please be sure your commit title and message follows the contribution guidelines described here -> https://github.com/eclipse-omr/omr/blob/master/CONTRIBUTING.md#commit-guidelines

@KavinSatheeskumar

KavinSatheeskumar commented Oct 1, 2025

Copy link
Copy Markdown
Contributor Author

I didn't review the contents of the PR (yet), but I have administrative feedback. Please be sure your commit title and message follows the contribution guidelines described here -> https://github.com/eclipse-omr/omr/blob/master/CONTRIBUTING.md#commit-guidelines

You can hold off on reviewing it for a bit, I discovered a few issues when testing. I am going to mark it as WIP for now and I will let you and Marius review after those issues are fixed.

@KavinSatheeskumar KavinSatheeskumar changed the title Make a number of small fixes to OMR to correct code cache segregation [WIP] Make a number of small fixes to OMR to correct code cache segregation Oct 1, 2025
@KavinSatheeskumar
KavinSatheeskumar marked this pull request as draft October 1, 2025 13:37
@KavinSatheeskumar
KavinSatheeskumar force-pushed the code_cache_segregation_fixes branch 2 times, most recently from 243532c to 8938eb6 Compare October 1, 2025 18:56
@KavinSatheeskumar

KavinSatheeskumar commented Oct 1, 2025

Copy link
Copy Markdown
Contributor Author

In order for PR eclipse-openj9/openj9#22625 to work correct (the PR which this supports), the flag retryCodeCacheAllocAndIgnoreKind needs to be enabled (otherwise a large number of methods get interpreted which don't need to be). In a future PR, this will be enabled by default.

@KavinSatheeskumar KavinSatheeskumar changed the title [WIP] Make a number of small fixes to OMR to correct code cache segregation Make a number of small fixes to OMR to correct code cache segregation Oct 1, 2025
@KavinSatheeskumar
KavinSatheeskumar marked this pull request as ready for review October 1, 2025 19:00
mpirvu
mpirvu reviewed Oct 2, 2025
View reviewed changes
Comment thread compiler/runtime/OMRCodeCacheManager.cpp Outdated
numCachesVisited++;
// Our current cache is reserved, so we cannot find it again
if (!codeCache->isReserved()) {
if (!codeCache->isReserved() && codeCache->_kind == kind) {

@mpirvu mpirvu Oct 2, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this if statement returns falls we are going to increment numCachesAlreadyReserved which is used later on.
The test on codeCache->_kind needs to stay on its own line (if statement)

@mpirvu

mpirvu commented Oct 2, 2025

Copy link
Copy Markdown
Contributor

Please modify the commit message to reflect the changes in this PR. You can/should make it slightly more verbose explaining what the bug was.
The title could be made more concise, e.g.: Fix code cache segregation logic

@KavinSatheeskumar KavinSatheeskumar changed the title Make a number of small fixes to OMR to correct code cache segregation Fix code cache segregation logic Oct 2, 2025
@KavinSatheeskumar
KavinSatheeskumar force-pushed the code_cache_segregation_fixes branch from 8938eb6 to 2ff97f3 Compare October 2, 2025 15:05
@KavinSatheeskumar
Fix code cache segregation logic
fb43869 
This patch updates the logic in allocateCodeMemoryWithRetries
to respect CodeCacheKind
@KavinSatheeskumar
KavinSatheeskumar force-pushed the code_cache_segregation_fixes branch from 2ff97f3 to fb43869 Compare October 2, 2025 18:41
mpirvu
mpirvu approved these changes Oct 2, 2025
View reviewed changes

@mpirvu mpirvu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@mpirvu

mpirvu commented Oct 6, 2025

Copy link
Copy Markdown
Contributor

This PR is ready for review/merge.

@dsouzai

dsouzai commented Oct 6, 2025

Copy link
Copy Markdown
Contributor

Does this code need to consider TR_RetryCodeCacheAllocAndIgnoreKind?

@KavinSatheeskumar

KavinSatheeskumar commented Oct 6, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Does this code need to consider TR_RetryCodeCacheAllocAndIgnoreKind?

No (we spent a pretty long time thinking about it), essentially what happens is that when we reserve the original code cache, on the first attempt it will be of kind TRANSIENT_CODE_CC, and if that fails, (which implies that no code cache of kind TRANSIENT_CODE_CC has enough space) it will ignore kind and find one of another type, usually DEFAULT_CC. This causes the code to be allocated in a code cache of kind DEFAULT_CC.

This behaviour has been confirmed in testing

@dsouzai

dsouzai commented Oct 6, 2025

Copy link
Copy Markdown
Contributor

jenkins build all

@dsouzai dsouzai self-assigned this Oct 6, 2025
@dsouzai

dsouzai commented Oct 6, 2025

Copy link
Copy Markdown
Contributor

ppc failure due to #6571
riscv failure due to #7938

@dsouzai
dsouzai merged commit 94469ec into eclipse-omr:master Oct 6, 2025
12 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dsouzai dsouzai dsouzai approved these changes
+1 more reviewer
@mpirvu mpirvu mpirvu approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@dsouzai dsouzai

Labels

comp:compiler

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@KavinSatheeskumar @0xdaryl @mpirvu @dsouzai