8369566: CRaC: Record metrics during checkpoint #269

rvansa · 2025-10-14T09:15:52Z

CRaC Engine can support storing additional metadata about the image. This can help the infrastructure to further refine the set of feasible images (constrained by CPU architecture and features) and select the image that is expected to perform best.

Progress

Change must not contain extraneous whitespace

Issue

JDK-8369566: CRaC: Record metrics during checkpoint (Enhancement - P4)

Reviewers

Timofei Pushkin (@TimPushkin - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/crac.git pull/269/head:pull/269
$ git checkout pull/269

Update a local copy of the PR:
$ git checkout pull/269
$ git pull https://git.openjdk.org/crac.git pull/269/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 269

View PR using the GUI difftool:
$ git pr show -t 269

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/crac/pull/269.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-10-14T09:17:15Z

👋 Welcome back rvansa! A progress list of the required criteria for merging this PR into crac will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-10-14T09:18:15Z

@rvansa This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8369566: CRaC: Record metrics during checkpoint

Reviewed-by: tpushkin

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the crac branch:

e2d4ede: 8369729: [CRaC] CRaC restore fails with different CPUs

Please see this link for an up-to-date comparison between the source branch of this pull request and the crac branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the crac branch, type /integrate in a new comment.

mlbridge · 2025-10-14T09:21:49Z

Webrevs

rvansa · 2025-10-14T10:41:27Z

It seems that the PathPatternTest introduced in #264 is unstable :-/

rvansa · 2025-10-16T08:13:24Z

@TimPushkin I've piggy-backed a fix in PathPatternTest, so now this is green. Could you give an ETA on review, please?

TimPushkin · 2025-10-16T08:17:07Z

@rvansa I'll try to look today, if the time allows; otherwise, I'll look tomorrow.

TimPushkin

There should be some way for the user to know which metrics are available because the names will be needed to write a selection policy. Ideally some documentation for the value ranges is also needed.

src/hotspot/share/prims/jvm.cpp

src/java.base/share/classes/jdk/internal/crac/Score.java

src/java.base/share/classes/jdk/internal/crac/mirror/impl/GlobalContext.java

src/hotspot/share/runtime/crac.cpp

src/java.base/share/native/libcrexec/image_score.hpp

TimPushkin · 2025-10-16T14:04:46Z

src/java.base/share/native/libcrexec/image_score.cpp

+    return false;
+  }
+  _score.foreach([&](const Score& score){
+    fprintf(f, "%s=%f\n", score._name, score._value);


There's no validation that = and \n are not present in the name so there will probably be problems with parsing

As for =, you can easily say that the last = is the separator. The \n is user shooting himself into foot.
However in general, this format (and its limitations) is engine-specific and there is no way to report the unsuitability. When the score is reported on Java level, the engine is not consulted yet, and later on we could only fail the checkpoint, which seems excessive to me.
If you want, I can truncate the metric name at first \n when recording it here.

I forgot that the value is not a generic string. With that = is indeed not a problem, for \n we can truncate with a warning.

src/java.base/share/native/libcrexec/image_score.hpp

rvansa · 2025-10-22T11:50:50Z

@TimPushkin I've added the test, and changed the way how the global context size is reported in scores: We have reported the practically constant version of internal global context, not the other (!) global context exposed to the user.

TimPushkin

I believe after our internal discussions we've decided to document the format of crexec's score output somewhere?

src/hotspot/share/include/crlib/crlib_image_score.h

src/java.base/share/native/libcrexec/crexec.hpp

src/java.base/share/native/libcrexec/hashtable.hpp

src/java.base/share/classes/jdk/internal/crac/mirror/impl/GlobalContext.java

TimPushkin · 2025-10-23T12:49:11Z

src/java.base/share/classes/jdk/internal/crac/Score.java

+    private static void setJdkResourceScore() {
+        int resources = 0;
+        for (var p : Core.Priority.values()) {
+            if (p.getContext() instanceof OrderedContext<?> octx) {


I would prefer a cast just to ensure we don't miss a resource, but for now this should be equivalent

Core.Priority.SCORE.getContext() is not an OrderedContext.

We could check p != Core.Priority.SCORE then. I am suggesting this because it'll make sure we won't miss resources if we add some other type of context in the future. But if you think this is unlikely, I'm OK to leave it as is.

src/java.base/share/native/libcrexec/README.md

rvansa · 2025-10-24T07:17:13Z

/integrate

openjdk · 2025-10-24T07:18:32Z

Going to push as commit 1bf79d7.
Since your change was applied there has been 1 commit pushed to the crac branch:

e2d4ede: 8369729: [CRaC] CRaC restore fails with different CPUs

Your commit was automatically rebased without conflicts.

openjdk · 2025-10-24T07:18:40Z

@rvansa Pushed as commit 1bf79d7.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8368929: CRaC: Record metrics during checkpoint

1fcc12a

openjdk bot added the rfr Pull request is ready for review label Oct 14, 2025

rvansa changed the title ~~8368929: CRaC: Record metrics during checkpoint~~ 8369566: CRaC: Record metrics during checkpoint Oct 14, 2025

openjdk bot added the ready Pull request is ready to be integrated label Oct 14, 2025

Don't use __typeof__

ab21851

Fix for PathPatternTest

523661a

rvansa requested a review from TimPushkin October 15, 2025 12:25

TimPushkin reviewed Oct 16, 2025

View reviewed changes

rvansa added 3 commits October 22, 2025 13:12

Update after review

e3a7d64

Don't record score for the internal global context

dba1ff1

Add license header

69cb158

TimPushkin reviewed Oct 23, 2025

View reviewed changes

Add README, truncate metric name, expose jdk.crac.internalResources

2ebee46

TimPushkin reviewed Oct 23, 2025

View reviewed changes

Update docs

cc331fd

TimPushkin approved these changes Oct 23, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Oct 24, 2025

openjdk bot closed this Oct 24, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 24, 2025

8369566: CRaC: Record metrics during checkpoint #269

8369566: CRaC: Record metrics during checkpoint #269

Conversation

rvansa commented Oct 14, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Oct 14, 2025

Uh oh!

openjdk bot commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

rvansa commented Oct 14, 2025

Uh oh!

rvansa commented Oct 16, 2025

Uh oh!

TimPushkin commented Oct 16, 2025

Uh oh!

TimPushkin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TimPushkin Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

rvansa Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

TimPushkin Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rvansa commented Oct 22, 2025

Uh oh!

TimPushkin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TimPushkin Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

rvansa Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

TimPushkin Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rvansa commented Oct 24, 2025

Uh oh!

openjdk bot commented Oct 24, 2025

Uh oh!

openjdk bot commented Oct 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

rvansa commented Oct 14, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Oct 14, 2025 •

edited

Loading

mlbridge bot commented Oct 14, 2025 •

edited

Loading

TimPushkin Oct 23, 2025 •

edited

Loading