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

Wed Feb 19 04:13:57 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 have added some comments mainly to understand the reason behind the changes when it was not clear to me.

src/hotspot/share/cds/archiveBuilder.cpp line 510:

> 508:   } else if (klass->is_objArray_klass()) {
> 509:     Klass* bottom = ObjArrayKlass::cast(klass)->bottom_klass();
> 510:     if (CDSConfig::is_dumping_dynamic_archive() && MetaspaceShared::is_shared_static(bottom)) {

Why do we have the check for `CDSConfig::is_dumping_dynamic_archive()` here?

src/hotspot/share/cds/archiveUtils.inline.hpp line 70:

> 68: // Returns the address of an Array<T> that's allocated in the ArchiveBuilder "buffer" space.
> 69: template <typename T>
> 70: Array<T>* ArchiveUtils::archive_ptr_array(GrowableArray<T>* tmp_array) {

If I am reading this code correctly it requires that the elements in `tmp_array` have already been archived. Can we add a comment and/or an assert to that effect.

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

> 548: 
> 549: bool CDSConfig::is_dumping_preimage_static_archive() {
> 550:   return _is_dumping_static_archive && _is_dumping_preimage_static_archive;

Is the check for `_is_dumping_static_archive` really needed?

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

> 703: bool CDSConfig::is_dumping_aot_linked_classes() {
> 704:   if (is_dumping_preimage_static_archive()) {
> 705:     return false;

In leyden-premain branch it returns `AOTClassLinking`, but here it is returning false. So we are not doing pre-linking in the preimage, is that right?

src/hotspot/share/cds/cppVtables.cpp line 197:

> 195: //     _orig_cpp_vtptrs[ConstantPool_Kind] ==  ((intptr_t**)cp)[0]
> 196: static intptr_t* _orig_cpp_vtptrs[_num_cloned_vtable_kinds];      // vtptrs set by the C++ compiler
> 197: static intptr_t* _archived_cpp_vtptrs[_num_cloned_vtable_kinds];  // vtptrs used in the static archive

what is the purpose of `_archived_cpp_vtptrs`?

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

> 1527:   // allow processes that have it open continued access to the file.
> 1528:   remove(_full_path);
> 1529:   int mode = CDSConfig::is_dumping_preimage_static_archive() ? 0666 : 0444;

Why do we need to give different access permission for preimage file compared to other dumping modes?

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

PR Review: https://git.openjdk.org/jdk/pull/23484#pullrequestreview-2625271355
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1960904061
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1960739673
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1960907271
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1960909728
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1960734314
PR Review Comment: https://git.openjdk.org/jdk/pull/23484#discussion_r1960916995