8353597: Refactor handling VM options for AOT cache input and output #24401

iklam · 2025-04-03T04:00:59Z

Since JEP 483: Ahead-of-Time Class Loading & Linking, VM options such as -XX:AOTCache are implemented as aliases of "classical" CDS options such as -XX:SharedArchiveFile.

In anticipation of the JEP: Ahead-of-time Command Line Ergonomics, we should refactor the code that deals with the AOT options. Specifically, as we expect the JVM to be able to load from an "input AOT cache" and write to an "output AOT cache", we should clearly identify the input and output caches in separate APIs:

const char* CDSConfig::input_static_archive_path();
const char* CDSConfig::input_dynamic_archive_path();
const char* CDSConfig::output_archive_path();

This PR also cleans up the code by:

renaming a few function to reflect what they actually do
moving more "config" management code into cdsConfig.cpp

There's also a behavioral bug fix: before this PR, -XX:AOTCache was handled by the ergo_init_classic_archive_paths() function, which allows two files to be specified. E.g., java -XX:AOTCache=static.jsa:dynamic.jsa. That's because -XX:AOTCache was implemented as an alias of -XX:SharedArchiveFile, and the latter allows this usage.

However, this behavior is not specified in JEP 483. Allowing two files in -XX:AOTCache will cause unnecessary complexity when we implement JDK-8353598: Allow AOT cache to be used in training run. Therefore, I added new test cases to disallow the use of two files. This also means that we don't need to modify the already over-complicated ergo_init_classic_archive_paths() for the AOT use cases

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8353597: Refactor handling VM options for AOT cache input and output (Enhancement - P3)

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer)
Ashutosh Mehra (@ashu-mehra - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24401

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24401.diff

Using Webrev

Link to Webrev Comment

…ilures

…ache

iklam · 2025-04-03T04:01:36Z

src/hotspot/share/cds/dynamicArchive.cpp

-    CDSConfig::disable_dumping_dynamic_archive();
-  }
-}
-


Moved to CDSConfig::prepare_for_dumping()

bridgekeeper · 2025-04-03T04:01:54Z

👋 Welcome back iklam! A progress list of the required criteria for merging this PR into pr/24272 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-04-03T04:02:28Z

@iklam 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:

8353597: Refactor handling VM options for AOT cache input and output

Reviewed-by: kvn, asmehra

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 19 new commits pushed to the master branch:

c3e0439: 8354121: Use a record class rather than a lambda in AbstractMemorySegmentImpl::cleanupAction
9d8b93b: 8354181: [Backout] 8334046: Set different values for CompLevel_any and CompLevel_all
da462cf: 8353692: Relax memory constraint on updating ObjectMonitorTable's item count
... and 16 more: https://git.openjdk.org/jdk/compare/c26c5758679b803489f401fbb23e8153ca10e19f...master

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 master branch, type /integrate in a new comment.

iklam · 2025-04-03T04:02:27Z

src/hotspot/share/cds/metaspaceShared.cpp

-void MetaspaceShared::prepare_for_dumping() {
-  assert(CDSConfig::is_dumping_archive(), "sanity");
-  CDSConfig::check_unsupported_dumping_module_options();
-}


Moved to CDSConfig::prepare_for_dumping()

openjdk · 2025-04-03T04:03:17Z

@iklam The following labels will be automatically applied to this pull request:

hotspot-runtime
serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-04-03T04:09:28Z

Webrevs

vnkozlov · 2025-04-03T15:56:02Z

@iklam one annoying thing in current ergonomic setting for AOTCode flags in mainline is checking which phase we are executing. We agreed before that we should only save/load AOT code when AOTClassLinking is on because AOT code needs classes to be preloaded.

I have to do next checks to enable AOTCode in CDSConfig::check_vm_args_consistency():

  if (AOTClassLinking && is_using_archive() && !is_dumping_archive() && !FLAG_IS_DEFAULT(AOTCache)) {
    FLAG_SET_ERGO_IF_DEFAULT(LoadAOTCode, true);
...
  if (AOTClassLinking && is_dumping_final_static_archive()) {
    FLAG_SET_ERGO_IF_DEFAULT(StoreAOTCode, true);

First, I am not sure these conditions are correct.

Second, it would be nice to have simple checks instead: is_dumping_aot_archive() and is_using_aot_archive().

May be also consider it is error if both conditions are true (we don't support updating archive yet).

vnkozlov · 2025-04-03T15:56:40Z

@veresov has also complex checks in his PR:
https://github.com/openjdk/jdk/pull/24210/files#diff-06d3a6d0bcf035b0b530313dc67b1ef7ef69869bcdee596a28f9efd97c86ace8

test/hotspot/jtreg/runtime/cds/appcds/AOTFlags.java

iklam · 2025-04-03T21:40:48Z

@iklam one annoying thing in current ergonomic setting for AOTCode flags in mainline is checking which phase we are executing. We agreed before that we should only save/load AOT code when AOTClassLinking is on because AOT code needs classes to be preloaded.

I have to do next checks to enable AOTCode in CDSConfig::check_vm_args_consistency():
  if (AOTClassLinking && is_using_archive() && !is_dumping_archive() && !FLAG_IS_DEFAULT(AOTCache)) {
    FLAG_SET_ERGO_IF_DEFAULT(LoadAOTCode, true);
...
  if (AOTClassLinking && is_dumping_final_static_archive()) {
    FLAG_SET_ERGO_IF_DEFAULT(StoreAOTCode, true);
First, I am not sure these conditions are correct.

Second, it would be nice to have simple checks instead: is_dumping_aot_archive() and is_using_aot_archive().

May be also consider it is error if both conditions are true (we don't support updating archive yet).

There are a lot of dependencies between different AOT capabilities, and it's hard to control that using global variables. At the point of CDSConfig::check_vm_args_consistency(), we don't have complete knowledge whether the AOT cache exists, or whether the cache contains AOT code, or whether the GC compressed oops settings are compatible with the AOT code.

In the handling of such "AOT capability flags", I have been using the following pattern:

In CDSConfig::check_vm_args_consistency() we update the default values of the flags according to their dependencies on other flags. E.g., by specifying -XX:AOTMode=create, AOTClassLinking and AOTInvokeDynamicLinking are enabled by default.

  if (!FLAG_IS_DEFAULT(AOTMode)) {
    // Using any form of the new AOTMode switch enables enhanced optimizations.
    FLAG_SET_ERGO_IF_DEFAULT(AOTClassLinking, true);
  }

  if (AOTClassLinking) {
    // If AOTClassLinking is specified, enable all AOT optimizations by default.
    FLAG_SET_ERGO_IF_DEFAULT(AOTInvokeDynamicLinking, true);
  } else {
    // AOTInvokeDynamicLinking depends on AOTClassLinking.
    FLAG_SET_ERGO(AOTInvokeDynamicLinking, false);
  }

However, the values of these flags are just advisory. Even if a flag is enabled, the underlying capability may be disabled. For example, AOTClassLinking requires the ability of dumping heap objects, which is not available if ZGC is used.

Because the dependencies are complex, it's difficult to resolve them statically and set a global boolean variable for each capability. Instead, I have been expressing the dependencies programmatically using accessor functions:

bool CDSConfig::is_dumping_aot_linked_classes() {
  if (is_dumping_preimage_static_archive()) {
    return false;
  } else if (is_dumping_dynamic_archive()) {
    return is_using_full_module_graph() && AOTClassLinking;
  } else if (is_dumping_static_archive()) {
    return is_dumping_full_module_graph() && AOTClassLinking;
  } else {
    return false;
  }
}

bool CDSConfig::is_dumping_invokedynamic() {
  // Requires is_dumping_aot_linked_classes(). Otherwise the classes of some archived heap
  // objects used by the archive indy callsites may be replaced at runtime.
  return AOTInvokeDynamicLinking && is_dumping_aot_linked_classes() && is_dumping_heap();
}

I would suggest doing something like this for storing AOT code:

bool CDSConfig::is_dumping_aot_code() {
    return StoreAOTCode && is_dumping_final_static_archive() && is_dumping_aot_linked_classes();
}

For loading AOT code, it's simpler. We can do a definite check immediately after the AOT cache has been mapped. This also makes the run-time check efficient (whereas the assembly-time checks can take their time).

 if (LoadAOTCode && cache has AOT code && vm options are compatible) {
    CDSConfig::_is_using_aot_code = true;
} else {
    CDSConfig::_is_using_aot_code = false;
}

inline bool CDSConfig::is_using_aot_code() {
    return CDSConfig::_is_using_aot_code;
}

vnkozlov · 2025-04-03T22:02:01Z

Thank you @iklam for explanation. I can do final adjustment to Store|LoadAOTCode flags values in StoreAOTCode::initialize() which is called from initialize_shared_spaces():

MetaspaceShared::initialize_shared_spaces() {
...
  static_mapinfo->patch_heap_embedded_pointers();
  ArchiveHeapLoader::finish_initialization();
  Universe::load_archived_object_instances();
+  AOTCodeCache::initialize();

The question: at this place are all CDS AOT flags are final (flags compatibility and cache presence are verified)?

Note, Store|LoadAOTCode flags are diagnostic and disabled by default. I need to set them to true somewhere.

iklam · 2025-04-06T21:27:50Z

Thank you @iklam for explanation. I can do final adjustment to Store|LoadAOTCode flags values in StoreAOTCode::initialize() which is called from initialize_shared_spaces():
MetaspaceShared::initialize_shared_spaces() {
...
  static_mapinfo->patch_heap_embedded_pointers();
  ArchiveHeapLoader::finish_initialization();
  Universe::load_archived_object_instances();
+  AOTCodeCache::initialize();
The question: at this place are all CDS AOT flags are final (flags compatibility and cache presence are verified)?

Note, Store|LoadAOTCode flags are diagnostic and disabled by default. I need to set them to true somewhere.

Yes, at this point all configuration related to AOT should be final. You can set the final values for the Store|LoadAOTCode flags here.

StoreAOTCode should be true only if CDSConfig::is_dumping_final_static_archive() is true. LoadAOTCode should be true only if CDSConfig::is_loading_archive() is true and the archive contains AOT code.

DanHeidinga · 2025-04-07T16:54:37Z

src/hotspot/share/cds/cdsConfig.cpp

+    // - SharedArchiveFile points to an archive that has failed CRC check
+    // - SharedArchiveFile is not specified and the VM doesn't have a compatible default archive
+
+#define __THEMSG " is unsupported when base CDS archive is not loaded. Run with -Xlog:cds for more info."


Do we want to start transitioning existing -Xlog:cds options to be :aot options? I think making the switch would match out long term direction

Yes, but I think we should do it only if AOTClassLinking is enabled. For legacy CDS we should continue use -Xlog:cds.
I am using -Xlog:aot+codecache in AOT code caching.

I created JDK-8354055 - Change "cds" logging tag to "aot". There are documentation/compatibility issues so we need to do some planning.

This particular block of code is moved from dynamicArchive.cpp to cdsConfig.cpp and I kept the logging tag the same.

Thanks @iklam. I agree with the approach of doing this in a separate issue.

vnkozlov

Looks good to me.

openjdk-notifier · 2025-04-09T01:20:55Z

The parent pull request that this pull request depends on has now been integrated and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork:

git checkout 8353597-refactor-aot-cache-input-output
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk · 2025-04-09T01:22:28Z

@iklam this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8353597-refactor-aot-cache-input-output
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

ashu-mehra

lgtm

vnkozlov

Re-approved.

iklam · 2025-04-09T15:01:15Z

Thanks @vnkozlov @ashu-mehra @DanHeidinga @lmesnik for the review
/integrate

openjdk · 2025-04-09T15:03:45Z

Going to push as commit 567c688.
Since your change was applied there have been 21 commits pushed to the master branch:

4dc9e58: 8310310: Migrate CreateSymbols tool in make/langtools to Classfile API
7a7b9ed: 8353727: HeapDumpPath doesn't expand %p
c3e0439: 8354121: Use a record class rather than a lambda in AbstractMemorySegmentImpl::cleanupAction
... and 18 more: https://git.openjdk.org/jdk/compare/c26c5758679b803489f401fbb23e8153ca10e19f...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-04-09T15:04:06Z

@iklam Pushed as commit 567c688.

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

iklam and others added 6 commits March 26, 2025 20:40

8353014: Exclude AOT tooling classes from AOT cache

9e214c5

Reverted some fixes in systemDictionaryShared.cpp that causes test fa…

13c8ae8

…ilures

Merge branch 'master' into 8353014-exclude-tooling-classes-from-aot-c…

a509649

…ache

Refactored CollectClassesForLinking for simplification

c994a2e

Clean up CDS input/output path handling

d01b253

Minimized changes in ergo_init_classic_archive_paths()

9e17fed

iklam commented Apr 3, 2025

View reviewed changes

iklam marked this pull request as ready for review April 3, 2025 04:02

openjdk bot added serviceability [email protected] hotspot-runtime [email protected] rfr Pull request is ready for review labels Apr 3, 2025

iklam changed the title ~~8353597 refactor aot cache input output~~ 8353597: Refactor handling VM options for AOT cache input and output Apr 3, 2025

more clean up

3ad42a3

lmesnik suggested changes Apr 3, 2025

View reviewed changes

test/hotspot/jtreg/runtime/cds/appcds/AOTFlags.java Outdated Show resolved Hide resolved

@lmesnik comments

90b4b68

vnkozlov mentioned this pull request Apr 3, 2025

8350209: Preserve adapters in AOT cache #24158

Draft

3 tasks

DanHeidinga reviewed Apr 7, 2025

View reviewed changes

vnkozlov approved these changes Apr 8, 2025

View reviewed changes

openjdk-notifier bot changed the base branch from pr/24272 to master April 9, 2025 01:20

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Apr 9, 2025

Merge branch 'master' into 8353597-refactor-aot-cache-input-output

e362524

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Apr 9, 2025

ashu-mehra approved these changes Apr 9, 2025

View reviewed changes

vnkozlov approved these changes Apr 9, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 9, 2025

openjdk bot added the integrated Pull request has been integrated label Apr 9, 2025

openjdk bot closed this Apr 9, 2025

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8353597: Refactor handling VM options for AOT cache input and output #24401

8353597: Refactor handling VM options for AOT cache input and output #24401

iklam commented Apr 3, 2025 •

edited by openjdk bot

Loading

iklam Apr 3, 2025

bridgekeeper bot commented Apr 3, 2025

openjdk bot commented Apr 3, 2025 •

edited

Loading

iklam Apr 3, 2025

openjdk bot commented Apr 3, 2025

mlbridge bot commented Apr 3, 2025 •

edited

Loading

vnkozlov commented Apr 3, 2025

vnkozlov commented Apr 3, 2025

iklam commented Apr 3, 2025 •

edited

Loading

vnkozlov commented Apr 3, 2025

iklam commented Apr 6, 2025

DanHeidinga Apr 7, 2025

vnkozlov Apr 8, 2025

iklam Apr 8, 2025

DanHeidinga Apr 8, 2025

vnkozlov left a comment

openjdk-notifier bot commented Apr 9, 2025

openjdk bot commented Apr 9, 2025

ashu-mehra left a comment

vnkozlov left a comment

iklam commented Apr 9, 2025

openjdk bot commented Apr 9, 2025

openjdk bot commented Apr 9, 2025

8353597: Refactor handling VM options for AOT cache input and output #24401

8353597: Refactor handling VM options for AOT cache input and output #24401

Conversation

iklam commented Apr 3, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

iklam Apr 3, 2025

Choose a reason for hiding this comment

bridgekeeper bot commented Apr 3, 2025

openjdk bot commented Apr 3, 2025 • edited Loading

iklam Apr 3, 2025

Choose a reason for hiding this comment

openjdk bot commented Apr 3, 2025

mlbridge bot commented Apr 3, 2025 • edited Loading

Webrevs

vnkozlov commented Apr 3, 2025

vnkozlov commented Apr 3, 2025

iklam commented Apr 3, 2025 • edited Loading

vnkozlov commented Apr 3, 2025

iklam commented Apr 6, 2025

DanHeidinga Apr 7, 2025

Choose a reason for hiding this comment

vnkozlov Apr 8, 2025

Choose a reason for hiding this comment

iklam Apr 8, 2025

Choose a reason for hiding this comment

DanHeidinga Apr 8, 2025

Choose a reason for hiding this comment

vnkozlov left a comment

Choose a reason for hiding this comment

openjdk-notifier bot commented Apr 9, 2025

openjdk bot commented Apr 9, 2025

ashu-mehra left a comment

Choose a reason for hiding this comment

vnkozlov left a comment

Choose a reason for hiding this comment

iklam commented Apr 9, 2025

openjdk bot commented Apr 9, 2025

openjdk bot commented Apr 9, 2025

iklam commented Apr 3, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Apr 3, 2025 •

edited

Loading

mlbridge bot commented Apr 3, 2025 •

edited

Loading

iklam commented Apr 3, 2025 •

edited

Loading