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

Remove CRIU Interpreter #20177

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

Conversation

ThanHenderson
Copy link
Contributor

@ThanHenderson ThanHenderson commented Sep 17, 2024

  • Remove CRIUBytcodeInterpreterCompressed
  • Remove CRIUBytcodeInterpreterFull
  • Add J9VM_CRIU_TRANSITION_TO_DEBUG_INTERPRETER flag
  • Add criuRestoreCheckForDebugInterpreterRequest hook
  • Set transition flag if -Xdump, -Xtrace, or JDWP is specified
  • checkTransitionToDebugInterpreter becomes infallible
  • Call checkTransitionToDebugInterpreter after restore hooks

Issues: #19835
Signed-off-by: Nathan Henderson [email protected]

@ThanHenderson
Copy link
Contributor Author

fyi @tajila. If we can merge #19754, I'll amend this patch with whatever is required w.r.t. #19835 (comment)

@ThanHenderson ThanHenderson added comp:vm criu Used to track CRIU snapshot related work labels Sep 17, 2024
runtime/vm/jvminit.c Outdated Show resolved Hide resolved
runtime/vm/CRIUHelpers.cpp Outdated Show resolved Hide resolved
runtime/vm/CRIUHelpers.cpp Outdated Show resolved Hide resolved
runtime/nls/j9vm/j9vm.nls Outdated Show resolved Hide resolved
@ThanHenderson
Copy link
Contributor Author

ThanHenderson commented Sep 19, 2024

I've added a criuRestoreCheckForJdwp hook that checks for -Xrunjdwp or -agentlib:jdwp= and sets J9VM_CRIU_TRANSITION_TO_DEBUG_INTERPRETER if either is present. I believe this is all that was alluded to by #19835 (comment). Let me know if there is any other work needing to be added here given #19754 has now been merged.

If we are good with the patch, I'll just need to add tests in test/functional/cmdLineTests/criu/src/org/openj9/criu/EnvVarFileTest.java and test/functional/cmdLineTests/criu/src/org/openj9/criu/OptionsFileTest.java for the JDWP options.

@ThanHenderson
Copy link
Contributor Author

Note: this will have to be double-delivered for the 0.48 release.

Copy link
Member

@JasonFengJ9 JasonFengJ9 left a comment

Choose a reason for hiding this comment

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

CRIU Interpreter was added to address a perf regression introduced by -Xtrace and -Xdump support via the restore option file.
It is better to run CRIU startup & first response benchmark tests to ensure no perf impact.

runtime/vm/CRIUHelpers.cpp Outdated Show resolved Hide resolved
runtime/vm/CRIUHelpers.cpp Outdated Show resolved Hide resolved
runtime/vm/CRIUHelpers.cpp Outdated Show resolved Hide resolved
@ThanHenderson
Copy link
Contributor Author

Passing CRIU functional tests: https://hyc-runtimes-jenkins.swg-devops.com/job/Grinder/43450/

@ThanHenderson
Copy link
Contributor Author

Here are some performance metrics:

pingperf

Baseline
StartupTime               avg= 358      min= 337        max= 402        stdDev=  12     maxVar=19.3%    confInt=0.63%   samples= 80
CRIUTime                  avg=  80      min=  79        max=  82        stdDev=   1     maxVar=4.8%     confInt=0.15%   samples= 73
Application               avg=  93      min=  91        max=  95        stdDev=   1     maxVar=4.4%     confInt=0.17%   samples= 80
FirstResponse             avg= 456      min= 435        max= 501        stdDev=  12     maxVar=15.2%    confInt=0.50%   samples= 80
ContainerFootprint (KiB)  avg=119331    min=118784      max=119808      stdDev= 260     maxVar=0.9%     confInt=0.04%   samples= 80
RSS (KiB)                 avg=149883    min=149712      max=150108      stdDev=  94     maxVar=0.3%     confInt=0.01%   samples= 79
Peak RSS (KiB)            avg=150058    min=147296      max=152232      stdDev=1055     maxVar=3.4%     confInt=0.13%   samples= 80

With patch
StartupTime               avg= 366      min= 341        max= 393        stdDev=  12     maxVar=15.2%    confInt=0.59%   samples= 80
CRIUTime                  avg=  81      min=  80        max=  86        stdDev=   1     maxVar=8.0%     confInt=0.29%   samples= 66
Application               avg=  94      min=  93        max=  97        stdDev=   1     maxVar=4.3%     confInt=0.19%   samples= 69
FirstResponse             avg= 463      min= 438        max= 487        stdDev=  11     maxVar=11.2%    confInt=0.46%   samples= 80
ContainerFootprint (KiB)  avg=121345    min=120934      max=121754      stdDev= 232     maxVar=0.7%     confInt=0.04%   samples= 80
RSS (KiB)                 avg=152370    min=152132      max=152684      stdDev= 119     maxVar=0.4%     confInt=0.01%   samples= 80
Peak RSS (KiB)            avg=151246    min=149556      max=153200      stdDev= 787     maxVar=2.4%     confInt=0.10%   samples= 80

restcrud

Baseline
StartupTime               avg= 406      min= 378        max= 446        stdDev=  15     maxVar=18.0%    confInt=0.67%   samples= 80
CRIUTime                  avg=  84      min=  84        max=  87        stdDev=   1     maxVar=3.6%     confInt=0.12%   samples= 71
Application               avg=  98      min=  96        max= 101        stdDev=   1     maxVar=5.2%     confInt=0.21%   samples= 79
FirstResponse             avg= 690      min= 643        max= 796        stdDev=  29     maxVar=23.8%    confInt=0.80%   samples= 79
ContainerFootprint (KiB)  avg=161331    min=160870      max=161894      stdDev= 236     maxVar=0.6%     confInt=0.03%   samples= 80
RSS (KiB)                 avg=211618    min=211360      max=212048      stdDev= 120     maxVar=0.3%     confInt=0.01%   samples= 80
Peak RSS (KiB)            avg=209741    min=208080      max=212292      stdDev= 846     maxVar=2.0%     confInt=0.08%   samples= 80

With Patch
StartupTime               avg= 409      min= 380        max= 441        stdDev=  12     maxVar=16.1%    confInt=0.54%   samples= 80
CRIUTime                  avg=  85      min=  84        max=  87        stdDev=   1     maxVar=3.7%     confInt=0.12%   samples= 73
Application               avg=  98      min=  97        max= 100        stdDev=   1     maxVar=3.1%     confInt=0.18%   samples= 80
FirstResponse             avg= 696      min= 646        max= 815        stdDev=  37     maxVar=26.2%    confInt=0.98%   samples= 80
ContainerFootprint (KiB)  avg=161295    min=160768      max=161894      stdDev= 238     maxVar=0.7%     confInt=0.03%   samples= 80
RSS (KiB)                 avg=212053    min=211752      max=212396      stdDev= 141     maxVar=0.3%     confInt=0.01%   samples= 80
Peak RSS (KiB)            avg=209384    min=205888      max=212740      stdDev=1769     maxVar=3.3%     confInt=0.16%   samples= 80

- Remove CRIUBytcodeInterpreterCompressed
- Remove CRIUBytcodeInterpreterFull
- Add J9VM_CRIU_TRANSITION_TO_DEBUG_INTERPRETER flag
- Add criuRestoreCheckForDebugInterpreterRequest hook
- Add criuRestoreCheckForJdwp hook
- Set transition flag if -Xdump, -Xtrace, or JDWP is specified
- checkTransitionToDebugInterpreter becomes infallible
- Call checkTransitionToDebugInterpreter after restore hooks

Issues: eclipse-openj9#19835
Signed-off-by: Nathan Henderson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:vm criu Used to track CRIU snapshot related work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants