RFR: 8348426: Generate binary file for -XX:AOTMode=record -XX:AOTConfiguration=file [v4]

Tue Feb 18 22:39:02 UTC 2025

On Thu, 13 Feb 2025 18:31:42 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> Currently, with `java -XX:AOTMode=record -XX:AOTConfiguration=file ...`, a text file is written. The file contains the names of loaded classes, indices of resolved constant pools entries, etc, that are easily represented in text.
>> 
>> With the upcoming 2nd JEP of the Leyden project, [JDK-8325147](https://bugs.openjdk.org/browse/JDK-8325147) (Ahead-of-Time Method Profiling), the AOT config file needs to record complex data structures that are difficult to represent in text (we would need code for serializing hierarchical data structures to/from text). Also, a next step after [JDK-8325147](https://bugs.openjdk.org/browse/JDK-8325147) would be to support hidden classes that have no predictable names. Representing such classes with textual names would become another challenge.
>> 
>> To prepare for  [JDK-8325147](https://bugs.openjdk.org/browse/JDK-8325147), this PR writes the AOT configuration file in a **binary format** (essentially the same format as a CDS archive file). This allows arbitrary data associated with the cached classes to be processed and stored using the existing `MetaspaceClosure` API (which can recursively copy C++ objects). Such a change in the file format is allowed by [JEP 483](https://openjdk.org/jeps/483):
>> 
>>> the format of the configuration and cache files is not specified and is subject to change without notice.
>> 
>> **Notes for reviewers:**
>> 
>> - Although the new config file format is essentially the same as a CDS "static" archive, for sanity, we use a different magic number so that the config file cannot be accidentally used as a CDS archive. See new tests inside AOTFlags.java.
>> - After this PR, the CDS "static" archive can be dumped in three modes: "classic", "preimage", and "final". See new comments in cdsConfig.hpp.
>> - The main starting point of this PR is `CDSConfig::check_aot_flags()` - it checks the existence of `-XX:AOTConfiguration` and `-XX:AOTMode` to configure the JVM to dump the CDS "preimage" or "final" archives as necessary.
>> - Most of the other changes are checks for `CDSConfig::is_dumping_preimage_static_archive()` and `CDSConfig::is_dumping_final_static_archive()` to handle subtlle differences between the different dumping modes.
>> - I also updated the UL messages to use the new JEP 483 terminology ("AOT cache", "AOT configuration file",  etc) when JEP 483 options are specified.
>> 
>> **Misc Note**
>> - The changes in [CDS.java and RunTests.gmk](https://github.com/iklam/jdk/commit/0e77a35c25a968c7d931931bc108ccb...
>
> Ioi Lam has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Improved JTREG_AOT_JDK=true so we do not need to add test code into the JDK itself
>  - Improve error message when AOTMode=create has an incompatible classpath

I spotted several minor items.

src/hotspot/share/cds/cdsConfig.cpp line 661:

> 659:   return UseSharedSpaces && is_using_full_module_graph() && _has_archived_invokedynamic;
> 660: }
> 661: 

Blank line removed by accident?

src/hotspot/share/cds/cdsConfig.hpp line 138:

> 136:   static void stop_using_full_module_graph(const char* reason = nullptr) NOT_CDS_JAVA_HEAP_RETURN;
> 137: 
> 138: 

Blank line revmoved by accident?

src/hotspot/share/cds/filemap.cpp line 1389:

> 1387:   const char* file_type = CDSConfig::type_of_archive_being_loaded();
> 1388:   if (_is_static) {
> 1389:     if ((gen_header->_magic == CDS_ARCHIVE_MAGIC) ||

Probably don't need the extra set of parentheses.

src/hotspot/share/cds/finalImageRecipes.hpp line 67:

> 65: 
> 66: public:
> 67:   static void serialize(SerializeClosure* soc, bool is_static_archive);

The only caller is from `MetaspaceShared::serialize(`):
    `FinalImageRecipes::serialize(soc, true);`
Wondering if the `is_static_archive` arg is needed?

src/hotspot/share/cds/metaspaceShared.cpp line 819:

> 817:   CDSConfig::DumperThreadMark dumper_thread_mark(THREAD);
> 818:   ResourceMark rm(THREAD);
> 819:   HandleMark hm(THREAD);

Why do we need HandleMark?

src/hotspot/share/cds/metaspaceShared.cpp line 839:

> 837:       tty->print_cr("AOTConfiguration recorded: %s", AOTConfiguration);
> 838:       vm_exit(0);
> 839:     } else {

Is it appropriate to add assert of `CDSConfig::is_dumping_final_static_archive()` in the `else` case?

src/hotspot/share/cds/metaspaceShared.cpp line 958:

> 956: 
> 957:   if (CDSConfig::is_dumping_preimage_static_archive()) {
> 958:     log_info(cds)("Reading lambda form invokers of in JDK default classlist ...");

Suggestion:
    "Reading lambda form invokers from JDK default classlist ...."

src/hotspot/share/classfile/systemDictionaryShared.cpp line 995:

> 993: 
> 994:   int length = record->num_verifier_constraints();
> 995:   if (length > 0 || klass->name()->equals("HelloWorld")) {

Is the "HelloWorld" check leftover from debugging?

src/hotspot/share/classfile/systemDictionaryShared.cpp line 1031:

> 1029: 
> 1030:   int length = rt_info->num_verifier_constraints();
> 1031:   if (length > 0 || klass->name()->equals("HelloWorld")) {

Is the "HelloWorld" check leftover from debugging?

src/hotspot/share/classfile/systemDictionaryShared.cpp line 1164:

> 1162:   JavaThread* current = JavaThread::current();
> 1163:   if (klass->is_shared_platform_class() || klass->is_shared_app_class()) {
> 1164:     DumpTimeClassInfo* dt_info = get_info(klass);

`dt_info` seems unused.

test/hotspot/jtreg/runtime/cds/appcds/aotClassLinking/AOTLoaderConstraintsTest.java line 80:

> 78:         public void checkExecution(OutputAnalyzer out, RunMode runMode) throws Exception {
> 79:             switch (runMode) {
> 80:             case RunMode.ASSEMBLY:   // JEP 485 + binary AOTConfiguration -- should load AppClass from preimage

s/485/483

test/hotspot/jtreg/runtime/cds/appcds/aotClassLinking/AOTLoaderConstraintsTest.java line 101:

> 99:         // AppClass is loaded by the app loader. To make sure that you cannot use
> 100:         // type masquerade attacks, we need to add a loader constraint that says:
> 101:         //  app and loo loaders must resolve the symbol "java/lang/String" to the same type.

Suggestion:

// app and _boot_ loaders ...

test/lib/jdk/test/lib/cds/CDSAppTester.java line 365:

> 363:     }
> 364: 
> 365:     // See JEP 485

s/485/483

-------------

PR Review: https://git.openjdk.org/jdk/pull/23484#pullrequestreview-2625174804
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1960672938
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1960673660
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1960675062
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1960684356
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1960676088
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1960677946
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1960679370
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1960680914
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1960681554
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1960682805
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1960686353
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1960687379
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1960685390