On Wed, 28 Oct 2026 02:30:56 GMT, Ashutosh Mehra <[email protected]> wrote:
>> Ioi Lam has updated the pull request with a new target base due to a merge >> or a rebase. The pull request now contains 55 commits: >> >> - cleaned up comment >> - Merge branch 'master' into aot-re-training >> - @vnkozlov comments -- check AOTReplayTraining >> - @vnkozlov comment -- removed unnecessary code >> - Renamed the CDSAppTester workflows to AOT-Iterative1 and AOT-Iterative2 >> - updated man page >> - Merge branch 'master' into aot-re-training >> - Avoid crashing inside ciRecords::Arguments<>::prepare() because >> _first==nullptr >> - Merge branch 'master' into aot-re-training >> - Fixed bug found in AOT/JCK testing >> - ... and 45 more: https://git.openjdk.org/jdk/compare/722e2fff...df2dd3ef > > src/hotspot/share/cds/archiveBuilder.cpp line 646: > >> 644: memcpy(dest, src, bytes); >> 645: if (CDSConfig::is_redumping_aot_configuration() && src_info->type() >> == MetaspaceClosureType::ClassType) { >> 646: // This class was aot-initialized in the input AOT cache, but we >> never aot-initialize classes in the > > Not every class is aot-initialized, but the comment and the check implies all > classes are aot-initialized and that when redumping the flag has to be > cleared. I changed to // This class might have been aot-initialized in the input AOT cache, but we never // aot-initialize classes in the output AOT configuration file. > src/hotspot/share/cds/archiveUtils.hpp line 221: > >> 219: }; >> 220: >> 221: // RAII helper for demarcating data structures that are loaded from the >> input archive in > > This looks more of a general purpose mechanism for disabling writing of > certain data structures, irrespective of which archive is being written. Just > that it is currently only used when redumping the config file. Perhaps the > comment can be tweaked a bit on these lines. I updated the comment and also renamed the function from `AOTWriteClosureDisabler::is_disable()` to `AOTWriteClosureDisabler::is_active()` to avoid using double-negatives. > src/hotspot/share/cds/cdsConfig.cpp line 790: > >> 788: assert(new_aot_flags_used(), ""); >> 789: >> 790: if (is_dumping_preimage_static_archive()) { > > Would it be better to use the new API `is_dumping_aot_configuration()`? Fixed > src/hotspot/share/cds/cdsConfig.cpp line 809: > >> 807: } >> 808: disable_dumping_aot_code(); // No AOT code generation during >> training run >> 809: } else if (is_dumping_final_static_archive()) { > > Same as above, should it be using new API `is_dumping_aot_cache()`? Fixed > src/hotspot/share/cds/dumpTimeClassInfo.inline.hpp line 65: > >> 63: } >> 64: >> 65: if (exclude_reason != nullptr && >> !!SystemDictionaryShared::is_excluded_class(k)) { > > extra `!` Thanks for catching this. If I remember correctly, the changes in this function were needed to handle the copying of unregistered classes from the old AOT cache. The `set_excluded_locked()` is for sanity only, so even if it is not executed here due to the `!!` typo, it shouldn't matter. In any case, I will rerun all tests just to make sure. > src/hotspot/share/cds/filemap.cpp line 233: > >> 231: FileMapInfo* FileMapInfo::allocate_dynamic_input_archive(const char* >> filename) { >> 232: precond(_dynamic_input_archive == nullptr); >> 233: _dynamic_input_archive = new FileMapInfo(filename, >> /*is_dynamic=*/false); > > Comment should be `is_static=` Fixed. > src/hotspot/share/runtime/sharedRuntime.cpp line 3453: > >> 3451: >> 3452: void >> AdapterHandlerLibrary::serialize_shared_table_header(SerializeClosure* soc) { >> 3453: // This table is used for loading cached adapters in a retaining >> JVM. However, > > typo: retaining -> retraining > there is also 2 spaces between "adapters" and "in". Fixed > src/hotspot/share/runtime/sharedRuntime.cpp line 3458: > >> 3456: AOTWriteClosureDisabler >> disable_if(CDSConfig::is_redumping_aot_configuration()); >> 3457: >> 3458: _aot_adapter_handler_table.serialize_header(soc); > > `serialize_header` writes a couple of ints and pointers. Pointers are taken > care of (null-ed out) when `AOTWriteClosureDisabler` is active but the ints > are not zeroed out. This can cause inconsistency in the archived header data. All writes by WriteClosure eventually go to here, which will write out a zero regardless of the input: void DumpRegion::append_intptr_t(intptr_t n) { if (AOTWriteClosureDisabler::is_active()) { n = 0; } > src/java.base/share/man/java.md line 4186: > >> 4184: The `-XX:AOTCache=<oldcache>` option is allowed when a JVM process >> is executed with the AOTMode >> 4185: of `record` (i.e., this JVM process is executing an AOT training >> run). If this option is specified, >> 4186: `<oldcache>` must be a valid AOT cache. The AOT configuration file >> written by this JVM process will > > This given an impression that only the profile data from the old cache is > used. But there can also be linked classes from the old cache which may not > otherwise be loaded in this training run. I changed the wording to `statistical data` to be consistent with the rest of the man page when describing the contents of the config file. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/31344#discussion_r4407576425 PR Review Comment: https://git.openjdk.org/jdk/pull/31344#discussion_r4407575384 PR Review Comment: https://git.openjdk.org/jdk/pull/31344#discussion_r4407574105 PR Review Comment: https://git.openjdk.org/jdk/pull/31344#discussion_r4407573941 PR Review Comment: https://git.openjdk.org/jdk/pull/31344#discussion_r4407573660 PR Review Comment: https://git.openjdk.org/jdk/pull/31344#discussion_r4407576681 PR Review Comment: https://git.openjdk.org/jdk/pull/31344#discussion_r4407571914 PR Review Comment: https://git.openjdk.org/jdk/pull/31344#discussion_r4407571507 PR Review Comment: https://git.openjdk.org/jdk/pull/31344#discussion_r4407570411
