Skip to content

Install AOT methods into cold code - #19976

Merged
dsouzai merged 1 commit into
eclipse-openj9:masterfrom
gita-omr:reinstall_aot_to_cold
Aug 19, 2024
Merged

Install AOT methods into cold code#19976
dsouzai merged 1 commit into
eclipse-openj9:masterfrom
gita-omr:reinstall_aot_to_cold

Conversation

@gita-omr

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

Copy link
Copy Markdown
Contributor
  • 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

dsouzai commented Aug 9, 2024

Copy link
Copy Markdown
Contributor

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

@dsouzai

dsouzai commented Aug 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

dsouzai commented Aug 12, 2024

Copy link
Copy Markdown
Contributor

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

gita-omr commented Aug 13, 2024

Copy link
Copy Markdown
Contributor Author

@vijaysun-omr @mpirvu fyi

mpirvu
mpirvu reviewed Aug 13, 2024
View reviewed changes
Comment thread runtime/compiler/runtime/RelocationRuntime.cpp Outdated
uint8_t *coldCode;
U_8 *codeStart = manager->allocateCodeMemory(codeSize, 0, &_codeCache, &coldCode, false);
U_8 *codeStart;
bool installIntoCold = TR::Options::getCmdLineOptions()->getOption(TR_ReinstallAOTToColdCode);

@mpirvu mpirvu Aug 13, 2024

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.

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

@gita-omr gita-omr Aug 14, 2024

Copy link
Copy Markdown
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

@mpirvu mpirvu Aug 14, 2024

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.

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

mpirvu commented Aug 14, 2024

Copy link
Copy Markdown
Contributor

I think local variables can have somewhat shortened names

ok, but is it actually re-installing AOT?

@gita-omr

gita-omr commented Aug 14, 2024

Copy link
Copy Markdown
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

gita-omr commented Aug 14, 2024

Copy link
Copy Markdown
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

dsouzai commented Aug 14, 2024

Copy link
Copy Markdown
Contributor

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

gita-omr commented Aug 14, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

gita-omr commented Aug 14, 2024

Copy link
Copy Markdown
Contributor Author

Renamed the option.

@dsouzai

dsouzai commented Aug 15, 2024

Copy link
Copy Markdown
Contributor

jenkins test sanity.functional all jdk21

@dsouzai

dsouzai commented Aug 16, 2024

Copy link
Copy Markdown
Contributor

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

pshipton commented Aug 16, 2024

Copy link
Copy Markdown
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

gita-omr commented Aug 16, 2024

Copy link
Copy Markdown
Contributor Author

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

@dsouzai

dsouzai commented Aug 16, 2024

Copy link
Copy Markdown
Contributor

jenkins test sanity.functional xlinux jdk21

@dsouzai

dsouzai commented Aug 19, 2024

Copy link
Copy Markdown
Contributor

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

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

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