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

Install AOT methods into cold code #19976

Merged
merged 1 commit into from
Aug 19, 2024
Merged

Install AOT methods into cold code #19976

merged 1 commit into from
Aug 19, 2024

Conversation

gita-omr
Copy link
Contributor

@gita-omr gita-omr commented Aug 7, 2024 •
edited
Loading

  • if TR_InstallAOTToCold option is on, install AOT methods into the cold part of the code cache

Depends on: eclipse-omr/omr#7440

@gita-omr gita-omr requested a review from dsouzai as a code owner August 7, 2024 20:17
@dsouzai dsouzai added comp:jit depends:omr Pull request is dependent on a corresponding change in OMR labels Aug 9, 2024
@dsouzai dsouzai self-assigned this Aug 9, 2024
@dsouzai
Copy link
Contributor

dsouzai commented Aug 9, 2024

jenkins test sanity.functional all jdk21 depends eclipse-omr/omr#7436

@dsouzai
Copy link
Contributor

dsouzai commented Aug 12, 2024 •
edited
Loading

aarch64 test failure looks to be due to #19845

win failure due to

All nodes of label ‘ci.role.build&&hw.arch.x86&&sw.os.windows’ are offline

@dsouzai
Copy link
Contributor

dsouzai commented Aug 12, 2024

Can't merge this yet as https://github.com/eclipse-openj9/openj9-omr/tree/openj9 does not yet have the necessary OMR changes.

@gita-omr
Copy link
Contributor Author

@vijaysun-omr @mpirvu fyi

mpirvu
mpirvu reviewed Aug 13, 2024
View reviewed changes
runtime/compiler/runtime/RelocationRuntime.cpp Outdated
@@ -1120,7 +1120,18 @@ TR_SharedCacheRelocationRuntime::allocateSpaceInCodeCache(UDATA codeSize)
}

uint8_t *coldCode;
U_8 *codeStart = manager->allocateCodeMemory(codeSize, 0, &_codeCache, &coldCode, false);
U_8 *codeStart;
bool installIntoCold = TR::Options::getCmdLineOptions()->getOption(TR_ReinstallAOTToColdCode);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the option called "reinstall"? Why not "install", especially that the variable is called installIntoCold?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think local variables can have somewhat shortened names. The option is already merged into OMR. Do you think it's worth renaming it? @mpirvu

Copy link
Contributor

Choose a reason for hiding this comment

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

You can open an issue to follow up, but in the interest of delivering this quicker it can stay like this for the time being.

@mpirvu
Copy link
Contributor

mpirvu commented Aug 14, 2024

I think local variables can have somewhat shortened names

ok, but is it actually re-installing AOT?

@gita-omr
Copy link
Contributor Author

I think local variables can have somewhat shortened names

ok, but is it actually re-installing AOT?

I actually don't know what is considered the right terminology. I searched through all AOT documents and did not find any mentioning of installing or re-installing.

@gita-omr
Copy link
Contributor Author

I think, in this situation, we already compiled the AOT method and it was already placed/installed into the code cache. Now, we installing it again. So "reinstall" seems to be appropriate or at least acceptable. If everybody agrees, I will keep the option and rename the variable. @mpirvu @dsouzai

@dsouzai
Copy link
Contributor

dsouzai commented Aug 14, 2024

I think, in this situation, we already compiled the AOT method and it was already placed/installed into the code cache. Now, we installing it again. So "reinstall" seems to be appropriate or at least acceptable.

I think the terminology has always been to store to the SCC and install into the code cache. Although we, during the AOT compile, generate code into the code cache, that isn't considered installing as the code in there isn't final (ie, it's not relocated) and so would not successfully execute.

If everybody agrees, I will keep the option and rename the variable.

I think it's fine for now, but the option should probably be changed at some point; I guess when I merged the OMR PR I wasn't careful enough about the name.

@gita-omr
Copy link
Contributor Author

gita-omr commented Aug 14, 2024 •
edited
Loading

Actually, let me just open a new OMR PR to get done with it.

@gita-omr gita-omr changed the title Reinstall AOT methods into cold code WIP: Reinstall AOT methods into cold code Aug 14, 2024
@gita-omr
Install AOT methods into cold code
65f2969 
- if TR_InstallAOTToCold option is on, install AOT methods
  into the cold part of the code cache
@gita-omr gita-omr force-pushed the reinstall_aot_to_cold branch from 691c590 to 65f2969 Compare August 14, 2024 15:55
@gita-omr gita-omr changed the title WIP: Reinstall AOT methods into cold code Install AOT methods into cold code Aug 14, 2024
@gita-omr
Copy link
Contributor Author

Renamed the option.

@dsouzai
Copy link
Contributor

dsouzai commented Aug 15, 2024

jenkins test sanity.functional all jdk21

@dsouzai
Copy link
Contributor

dsouzai commented Aug 16, 2024

windows failed because of the usual

All nodes of label ‘ci.role.build&&hw.arch.x86&&sw.os.windows’ are offline

The xlinux failures all seem to be related to being unable to create a dump.

@pshipton
Copy link
Member

The xlinux failures all seem to be related to being unable to create a dump.

There was a configuration problem on ub22x64j95 which should be resolved now.

@gita-omr
Copy link
Contributor Author

The failures seems to be unrelated, especially since the code is only enabled under an option.

@dsouzai
Copy link
Contributor

dsouzai commented Aug 16, 2024

jenkins test sanity.functional xlinux jdk21

@dsouzai
Copy link
Contributor

dsouzai commented Aug 19, 2024

All (relevant) tests passed; will wait until @mpirvu's OK.

mpirvu
mpirvu approved these changes Aug 19, 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

@dsouzai dsouzai merged commit 1ffcc7a into eclipse-openj9:master Aug 19, 2024
15 of 16 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

@mpirvu mpirvu mpirvu approved these changes

Assignees

@dsouzai dsouzai

Labels
comp:jit depends:omr Pull request is dependent on a corresponding change in OMR
Projects
None yet
Milestone
Release 0.48 (Java 8, 11, 17, 21, 23)...
Development

Successfully merging this pull request may close these issues.
4 participants
@gita-omr @dsouzai @mpirvu @pshipton