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

Close and Reopen JIT log files across a checkpoint/restore #20935

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Close and Reopen JIT log files across a checkpoint/restore #20935

wants to merge 1 commit into from

Conversation

dsouzai
Copy link
Contributor

@dsouzai dsouzai commented Jan 14, 2025

A common best practice in CRIU mode, at least in OpenJ9, is to close all open files before a checkpoint, and reopen them on restore. The JIT would keep log files such as the vlog and rtLog open across a checkpoint/restore boundary. This PR closes these files on checkpoint and reopens them on restore.

@dsouzai dsouzai added comp:jit criu Used to track CRIU snapshot related work labels Jan 14, 2025
@dsouzai
Copy link
Contributor Author

dsouzai commented Jan 14, 2025

A consequence of this change is that when disableSuffixLogs is not specified, a different file is opened. However, this is consistent with what happens for example in the GC.

@dsouzai
Copy link
Contributor Author

dsouzai commented Jan 17, 2025

@mpirvu could you please review?

@mpirvu mpirvu self-assigned this Jan 17, 2025
mpirvu
mpirvu reviewed Jan 17, 2025
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.

Is openNewRTLog(), openNewVlog() and openLogFilesIfNeeded() going to be used anymore?
Also, what about the compilation logs?

runtime/compiler/runtime/CRRuntime.cpp Show resolved Hide resolved
@dsouzai
Copy link
Contributor Author

dsouzai commented Jan 17, 2025

Also, what about the compilation logs?

I believe these get opened at the start of a compilation and are closed at the end of one.

@dsouzai
Copy link
Contributor Author

dsouzai commented Jan 17, 2025

One thing I"m not sure about actually is what happens right now if we specify disableSuffixLogs; I think as the code stands, it'll overwrite the existing file, which is not what we want. So I may need to look into that.

@dsouzai dsouzai marked this pull request as draft January 17, 2025 18:03
@mpirvu
Copy link
Contributor

mpirvu commented Jan 17, 2025

Do you to handle disableSuffixLogs in this PR or separately?

@dsouzai
Copy link
Contributor Author

dsouzai commented Jan 17, 2025

Do you to handle disableSuffixLogs in this PR or separately?

I figure I should handle it here; I think otherwise, it'll break some of those cmdLineTester tests.

@dsouzai
Close and Reopen log files across a checkpoint/restore
7a3856d 
A common best practice in CRIU mode, at least in OpenJ9, is to close all
open files before a checkpoint, and reopen them on restore. The JIT
would keep log files such as the vlog and rtLog open across a
checkpoint/restore boundary. This commit closes these files on
checkpoint and reopens them on restore.

Signed-off-by: Irwin D'Souza <[email protected]>
@dsouzai
Copy link
Contributor Author

dsouzai commented Jan 17, 2025

Also, what about the compilation logs?

I believe these get opened at the start of a compilation and are closed at the end of one.

I thought this was the case, but it may not be the case... I think we hold on to the file handles so that they can be used by different threads. I'll have to look deeper into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpirvu mpirvu mpirvu left review comments

Assignees

@mpirvu mpirvu

Labels
comp:jit criu Used to track CRIU snapshot related work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dsouzai @mpirvu