On Fri, 25 Apr 2025 20:18:41 GMT, Igor Veresov <ivere...@openjdk.org> wrote:
> Improve warm-up time by making profile data from a previous run of an > application instantly available, when the HotSpot Java Virtual Machine > starts. Specifically, enhance the [AOT cache](https://openjdk.org/jeps/483) > to store method execution profiles from training runs, reducing profiling > delays in subsequent production runs. > > More details in the JEP: https://bugs.openjdk.org/browse/JDK-8325147 In general it is okay. Please, use UL with `(aot, training)` tags for your code. I noticed (and added comment) that you use `guarantee()` which crashes VM when you call new `verify` methods. Can you disable TD instead and continue execution? src/hotspot/share/cds/archiveBuilder.cpp line 770: > 768: relocate_embedded_pointers(&_rw_src_objs); > 769: relocate_embedded_pointers(&_ro_src_objs); > 770: log_info(cds)("Relocating %zu pointers, %zu tagged, %zu nulled", `log_info(aot)` if it is Leyden related. src/hotspot/share/cds/dumpAllocStats.hpp line 151: > 149: } > 150: > 151: void record_dynamic_proxy_class() { This is not called. This code seems not related. src/hotspot/share/ci/ciInstanceKlass.hpp line 47: > 45: friend class ciField; > 46: friend class ciReplay; > 47: friend class CompileTrainingData; Not referenced here src/hotspot/share/ci/ciMethod.cpp line 1147: > 1145: // heuristic (e.g. post call nop instructions; see > InlineSkippedInstructionsCounter) > 1146: int ciMethod::inline_instructions_size() { > 1147: if (_inline_instructions_size == -1) { Why repeat this condition and not put new code under existing one? src/hotspot/share/ci/ciMethodData.cpp line 71: > 69: > 70: bool is_live(Method* m) { > 71: Klass* holder = m->method_holder(); Changes in this file seems not related and can be pushed/tested separately. If they are related - there should be condition for additional checks. src/hotspot/share/ci/ciObjectFactory.hpp line 2: > 1: /* > 2: * Copyright (c) 1999, 2020, Oracle and/or its affiliates. All rights > reserved. Wrong year src/hotspot/share/compiler/compileBroker.hpp line 456: > 454: }; > 455: > 456: class TrainingReplayThread : public JavaThread { Add comment explaining what this thread do exactly. src/hotspot/share/memory/allocation.cpp line 89: > 87: } > 88: > 89: // Work-around -- see JDK-8331086 We should not use bug ID in comment (here and in .hpp file). Please, explain in the comment why you do that. src/hotspot/share/oops/methodCounters.cpp line 57: > 55: > 56: MethodCounters::MethodCounters() { > 57: assert(CDSConfig::is_dumping_static_archive() || UseSharedSpaces, "only > for CDS"); The same comment as for `MathodData()`. src/hotspot/share/oops/methodCounters.cpp line 82: > 80: > 81: void MethodCounters::metaspace_pointers_do(MetaspaceClosure* it) { > 82: log_trace(cds)("Iter(MethodCounters): %p", this); Use `log_trace(aot, training)` src/hotspot/share/oops/methodCounters.hpp line 129: > 127: void set_highest_osr_comp_level(int level) { > _highest_osr_comp_level = (u1)level; } > 128: > 129: Extra empty line src/hotspot/share/oops/methodData.cpp line 1296: > 1294: > 1295: MethodData::MethodData() { > 1296: assert(CDSConfig::is_dumping_static_archive() || UseSharedSpaces, > "only for CDS"); 1. Should its code be guarded by `#if INCLUDE_CDS`? 2. Comment where/how it is used. 3. Is it used in all phases or only during TRAINING and ASSEMBLY? 4. Can you add query methods into `CDSConfig` which you can call here and in other places?: is_dumping_training_data() is_using_training_data() src/hotspot/share/oops/methodData.cpp line 1434: > 1432: > 1433: bool MethodData::is_mature() const { > 1434: return CompilationPolicy::is_mature((MethodData*)this); Why you need the cast? src/hotspot/share/oops/methodData.cpp line 1796: > 1794: > 1795: void MethodData::metaspace_pointers_do(MetaspaceClosure* it) { > 1796: log_trace(cds)("Iter(MethodData): %p for %p %s", this, _method, > _method->name_and_sig_as_C_string()); We discussed yesterday and we need to use `aot` instead `cds` for Leyden. `aot` tag is already in mainline. I suggest to use new tag `(aot, training)` to separate your output from general CDS/AOT output. src/hotspot/share/oops/trainingData.cpp line 2: > 1: /* > 2: * Copyright (c) 2023, 2025, Oracle and/or its affiliates. All rights > reserved. Use one year src/hotspot/share/oops/trainingData.cpp line 54: > 52: > 53: MethodTrainingData::MethodTrainingData() { > 54: assert(CDSConfig::is_dumping_static_archive() || UseSharedSpaces, "only > for CDS"); Consider adding and using `CDSConfig::is_dumping_training_data() ` or something. src/hotspot/share/oops/trainingData.cpp line 76: > 74: > 75: static void verify_archived_entry(TrainingData* td, const > TrainingData::Key* k) { > 76: guarantee(TrainingData::Key::can_compute_cds_hash(k), ""); Should we gracefully disable using TD instead of crashing VM? src/hotspot/share/oops/trainingData.cpp line 545: > 543: if (is_excluded) { > 544: ResourceMark rm; > 545: log_debug(cds)("Cleanup KTD %s", name()->as_klass_external_name()); `log_debug(aot, training)` src/hotspot/share/oops/trainingData.hpp line 2: > 1: /* > 2: * Copyright (c) 2023, 2025, Oracle and/or its affiliates. All rights > reserved. This is debatable but suggestion to use only one year (2025) for new files. src/hotspot/share/oops/trainingData.hpp line 756: > 754: int highest_level() const { return highest_level(_level_mask); > } > 755: int highest_top_level() const { return _highest_top_level; } > 756: MethodData* final_profile() const { return _final_profile; } `never_inlined()`, `highest_level()` are not used. src/hotspot/share/runtime/init.cpp line 189: > 187: #endif > 188: > 189: if (TrainingData::have_data() || TrainingData::need_data()) { Why 2 checks? Comment please. test/hotspot/jtreg/runtime/cds/appcds/aotProfile/AOTProfileFlags.java line 30: > 28: * @requires vm.cds > 29: * @comment work around JDK-8345635 > 30: * @requires !vm.jvmci.enabled Consider adding: * @requires vm.cds.supports.aot.class.linking * @requires vm.flagless ------------- Changes requested by kvn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/24886#pullrequestreview-2796434540 PR Review Comment: https://git.openjdk.org/jdk/pull/24886#discussion_r2061680543 PR Review Comment: https://git.openjdk.org/jdk/pull/24886#discussion_r2061639984 PR Review Comment: https://git.openjdk.org/jdk/pull/24886#discussion_r2061668601 PR Review Comment: https://git.openjdk.org/jdk/pull/24886#discussion_r2061637565 PR Review Comment: https://git.openjdk.org/jdk/pull/24886#discussion_r2061635488 PR Review Comment: https://git.openjdk.org/jdk/pull/24886#discussion_r2061668094 PR Review Comment: https://git.openjdk.org/jdk/pull/24886#discussion_r2061631348 PR Review Comment: https://git.openjdk.org/jdk/pull/24886#discussion_r2061629193 PR Review Comment: https://git.openjdk.org/jdk/pull/24886#discussion_r2061626756 PR Review Comment: https://git.openjdk.org/jdk/pull/24886#discussion_r2061626460 PR Review Comment: https://git.openjdk.org/jdk/pull/24886#discussion_r2061626301 PR Review Comment: https://git.openjdk.org/jdk/pull/24886#discussion_r2061626021 PR Review Comment: https://git.openjdk.org/jdk/pull/24886#discussion_r2061625757 PR Review Comment: https://git.openjdk.org/jdk/pull/24886#discussion_r2061625222 PR Review Comment: https://git.openjdk.org/jdk/pull/24886#discussion_r2061687468 PR Review Comment: https://git.openjdk.org/jdk/pull/24886#discussion_r2061690598 PR Review Comment: https://git.openjdk.org/jdk/pull/24886#discussion_r2061694717 PR Review Comment: https://git.openjdk.org/jdk/pull/24886#discussion_r2061697540 PR Review Comment: https://git.openjdk.org/jdk/pull/24886#discussion_r2061627798 PR Review Comment: https://git.openjdk.org/jdk/pull/24886#discussion_r2061664186 PR Review Comment: https://git.openjdk.org/jdk/pull/24886#discussion_r2061612509 PR Review Comment: https://git.openjdk.org/jdk/pull/24886#discussion_r2061611330
