RFR: 8355003: Implement Ahead-of-Time Method Profiling
Vladimir Kozlov
kvn at openjdk.org
Sat Apr 26 22:49:52 UTC 2025
On Fri, 25 Apr 2025 20:18:41 GMT, Igor Veresov <iveresov at 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
More information about the hotspot-compiler-dev
mailing list