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

Add support to disable JITServer and AOT post restore #16911

Merged
merged 2 commits into from
Mar 15, 2023

Conversation

dsouzai
Copy link
Contributor

@dsouzai dsouzai commented Mar 14, 2023

Since JITServer is enabled implicitly in Non Portable Mode, specifying -XX:-UseJITServer should disable it post-restore.

Additionally, if -Xshareclasses:disableOnRestore is specified, disable AOT (see #16868).

@dsouzai dsouzai added comp:jit comp:jitserver Artifacts related to JIT-as-a-Service project criu Used to track CRIU snapshot related work labels Mar 14, 2023
@dsouzai
Copy link
Contributor Author

dsouzai commented Mar 14, 2023

@mpirvu could you please review?

@dsouzai dsouzai mentioned this pull request Mar 14, 2023
30 tasks
@dsouzai dsouzai marked this pull request as draft March 14, 2023 17:20
@dsouzai
Copy link
Contributor Author

dsouzai commented Mar 14, 2023

Converting to draft so I can also add the change to support disabling the SCC post restore in this PR (to minimize the amount of double delivering needed before Friday).

@@ -261,6 +263,13 @@ J9::OptionsPostRestore::iterateOverExternalOptions()
}
break;

case J9::ExternalOptions::XShareclassesDisableOnRestore:
{
if (FIND_ARG_IN_RESTORE_ARGS(OPTIONAL_LIST_MATCH, optString, 0) >= 0)
Copy link
Contributor Author

@dsouzai dsouzai Mar 14, 2023

Choose a reason for hiding this comment

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

At the moment, running with -Xshareclasses:disableOnRestore will fail on restore (unless I change this to FIND_AND_CONSUME_RESTORE_ARG). I'm expecting the VM to use the FIND_AND_CONSUME variant when they process this option.

fyi @hangshao0

@dsouzai dsouzai changed the title Add support to disable JITServer post restore Add support to disable JITServer and AOT post restore Mar 14, 2023
@dsouzai dsouzai marked this pull request as ready for review March 14, 2023 19:29
@dsouzai
Copy link
Contributor Author

dsouzai commented Mar 14, 2023

@mpirvu ready for review now.

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 wonder if we should reverse the logic for JITServer: run in client mode before checkpoint (as we do now), but by default disable client-mode post restore, unless the user specifies -XX:+UseJITServer in which case we let the JVM in client mode.
This would be more in line with current behavior where the user has to opt-in to use JITServer.

@mpirvu
Copy link
Contributor

mpirvu commented Mar 15, 2023

jenkins test sanity all jdk17

@dsouzai
Copy link
Contributor Author

dsouzai commented Mar 15, 2023

I wonder if we should reverse the logic for JITServer: run in client mode before checkpoint (as we do now), but by default disable client-mode post restore, unless the user specifies -XX:+UseJITServer in which case we let the JVM in client mode.
This would be more in line with current behavior where the user has to opt-in to use JITServer.

It's something we can look into for post 0.38; I have a task in #16853 under the JITServer section to support -XX:+UseJITServer post restore in Portable mode which maybe could make use of this same idea.

That said, I believe having JITServer enabled implicitly in Non-Portable mode was a deliberate decision, though it was made before we had the ability to specify options post-restore, so it could be something to revisit. @vijaysun-omr @ymanton what are your thoughts on Marius' suggestion (since you guys made the decision to implicitly enable JITServer).

@vijaysun-omr
Copy link
Contributor

Yes I think the prior decision was driven by a desire to perhaps attempt a demo with both JIT server and CRIU support, but in a world where we were unable to specify new JVM command line options on the restore side. Now that that has changed, I agree that we could consider a different design.

@mpirvu mpirvu merged commit 5938c2f into eclipse-openj9:master Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:jit comp:jitserver Artifacts related to JIT-as-a-Service project criu Used to track CRIU snapshot related work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants