On Wed, 8 Jul 2026 18:06:01 GMT, Andrew Haley <[email protected]> wrote:
>> Please use [this >> link](https://github.com/openjdk/jdk/pull/28541/changes?w=1) to view the >> files changed. >> >> Profile counters scale very badly. >> >> The overhead for profiled code isn't too bad with one thread, but as the >> thread count increases, things go wrong very quickly. >> >> For example, here's a benchmark from the OpenJDK test suite, run at >> TieredLevel 3 with one thread, then three threads: >> >> >> Benchmark (randomized) Mode Cnt Score Error Units >> InterfaceCalls.test2ndInt5Types false avgt 4 27.468 ± 2.631 ns/op >> InterfaceCalls.test2ndInt5Types false avgt 4 240.010 ± 6.329 ns/op >> >> >> This slowdown is caused by high memory contention on the profile counters. >> Not only is this slow, but it can also lose profile counts. >> >> This patch is for C1 only. It'd be easy to randomize C1 counters as well in >> another PR, if anyone thinks it's worth doing. >> >> One other thing to note is that randomized profile counters degrade very >> badly with small decimation ratios. For example, using a ratio of 2 with >> `-XX:ProfileCaptureRatio=2` with a single thread results in >> >> >> Benchmark (randomized) Mode Cnt Score Error >> Units >> InterfaceCalls.test2ndInt5Types false avgt 4 80.147 ± 9.991 >> ns/op >> >> >> The problem is that the branch prediction rate drops away very badly, >> leading to many mispredictions. It only really makes sense to use higher >> decimation ratios, e.g. 64. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Andrew Haley has updated the pull request incrementally with one additional > commit since the last revision: > > More Some audit findings: src/hotspot/cpu/aarch64/assembler_aarch64.hpp line 153: > 151: > 152: // State for randomized profile counters. Used by C1. > 153: extern Register r_profile_rng; So if it is C1-only, should it _be_ in C1-specific assembler somewhere? Or we would not be able to reference it from any shared code that is used by C1? src/hotspot/cpu/aarch64/c1_FrameMap_aarch64.hpp line 130: > 128: static LIR_Opr fpu0_double_opr; > 129: > 130: // static LIR_Opr r_profile_rng_opr; Leftover? src/hotspot/cpu/aarch64/c1_LIRAssembler_aarch64.cpp line 2562: > 2560: > 2561: #ifndef PRODUCT > 2562: long tier3_overflows; This going to collide with something eventually? Not sure what to do with it, maybe it belongs in `LIR_Assembler`... src/hotspot/cpu/aarch64/c1_LIRAssembler_aarch64.cpp line 2629: > 2627: } > 2628: case T_LONG: { > 2629: inc *= ProfileCaptureRatio; So you already did this increment at L2620 above. This multiplies the increment by PCR^2? src/hotspot/cpu/aarch64/c1_MacroAssembler_aarch64.cpp line 285: > 283: cbnzw(r_profile_rng, not_zero); > 284: stop("non-zero required before step"); > 285: bind(not_zero); Want to guard this with `ASSERT`? Also I suspect we can legitimately have zero as LCG state, so it can fire in product builds? src/hotspot/cpu/x86/c1_LIRAssembler_x86.cpp line 1294: > 1292: assert(!dst.uses(temp), "fix register allocation"); > 1293: auto threshold = (UCONST64(1) << 32) >> ratio_shift; > 1294: __ cmpl(r_profile_rng, threshold); Something smells funky here. `threshold` ends up being `uint64_t`? Want to `check_cast` it to `int32_t` that `cmpl` wants? src/hotspot/cpu/x86/c1_LIRAssembler_x86.cpp line 2906: > 2904: __ movl(step_opr->as_register(), > 2905: InvocationCounter::count_increment * > ProfileCaptureRatio); > 2906: __ cmovl(Assembler::equal, dest, step_opr->as_register()); Hold on, this clobbers `step_opr` unconditionally? I see there is a use of `step_opr` later at L2917, so it is still live? AArch64 does `rscratch1` here, so the intent is to have a temp of sorts? src/hotspot/share/c1/c1_LIRGenerator.cpp line 3222: > 3220: > MethodData::invocation_counter_offset()); > 3221: counters_base = LIR_OprFact::metadataConst > 3222: (method->method_data()->constant_encoding()); The existing method just below does: ciMethodData* md = method->method_data_or_null(); assert(md != nullptr, "Sanity"); __ metadata2reg(md->constant_encoding(), counter_holder); Do the same here? Null-checks the MDO... src/hotspot/share/ci/ciEnv.cpp line 1074: > 1072: compiler, > CompLevel(task()->comp_level()), > 1073: nmethod::Flags(has_unsafe_access, > has_wide_vectors, has_monitors, has_scoped_access)); > 1074: { I don't see why this move is needed? src/hotspot/share/compiler/compilationPolicy.cpp line 415: > 413: void CompilationPolicy::print_event_on(outputStream *st, EventType type, > Method* m, Method* im, int bci, CompLevel level) { > 414: > 415: if (level < CompLevel_full_optimization) return; Debugging leftover? src/hotspot/share/compiler/compilationPolicy.cpp line 506: > 504: } > 505: > 506: if (im) { Should be `im != nullptr`. Also, I think there are methods with null MDOs, so need to null-check `im->method_data()` as well? src/hotspot/share/utilities/debug.cpp line 380: > 378: cb->print(); > 379: } > 380: // Disassembler::decode(cb); Debugging leftover. src/hotspot/share/utilities/vmError.cpp line 1641: > 1639: va_start(detail_args, detail_fmt); > 1640: fprintf(stderr, "Spinning\n"); fflush(stderr); > 1641: for(;;); Spinloop goes brrrrrrrrrrrrrrr test/hotspot/jtreg/compiler/tiered/LevelTransitionTest.java line 42: > 40: * compiler.tiered.LevelTransitionTest > 41: * > 42: * @requires os.arch=="amd64" | os.arch=="x86_64" | os.arch=="arm" | > os.arch=="aarch64" Wait, this ANDs with the existing test config. So it effectively disables the test everywhere except these platforms. You need to split out (copy-paste) the whole block starting from `/** ... @test`. ------------- PR Review: https://git.openjdk.org/jdk/pull/28541#pullrequestreview-4664029003 PR Review Comment: https://git.openjdk.org/jdk/pull/28541#discussion_r3552669676 PR Review Comment: https://git.openjdk.org/jdk/pull/28541#discussion_r3552678052 PR Review Comment: https://git.openjdk.org/jdk/pull/28541#discussion_r3552605952 PR Review Comment: https://git.openjdk.org/jdk/pull/28541#discussion_r3552612306 PR Review Comment: https://git.openjdk.org/jdk/pull/28541#discussion_r3552547932 PR Review Comment: https://git.openjdk.org/jdk/pull/28541#discussion_r3552644177 PR Review Comment: https://git.openjdk.org/jdk/pull/28541#discussion_r3552585098 PR Review Comment: https://git.openjdk.org/jdk/pull/28541#discussion_r3552656783 PR Review Comment: https://git.openjdk.org/jdk/pull/28541#discussion_r3552621581 PR Review Comment: https://git.openjdk.org/jdk/pull/28541#discussion_r3552522716 PR Review Comment: https://git.openjdk.org/jdk/pull/28541#discussion_r3552516374 PR Review Comment: https://git.openjdk.org/jdk/pull/28541#discussion_r3552591598 PR Review Comment: https://git.openjdk.org/jdk/pull/28541#discussion_r3552506338 PR Review Comment: https://git.openjdk.org/jdk/pull/28541#discussion_r3552531020
