On Fri, 17 Apr 2026 13:11:43 GMT, Aleksey Shipilev <[email protected]> wrote:
> I can take a look at Shenandoah parts, if you want. We probably just missing
> a few C1 stubs for it?
Please do. Look on `AOTCodeAddressTable::set_c1_stubs_complete()` where they
are recorded.
> If you could merge from master, I can kick off another code analyzer run.
Merged
@shipilev please look on GHA failure in
`runtime/cds/appcds/aotCode/AOTCodeTest.java#shenandoah` test after merge from
mainline.
> src/hotspot/cpu/x86/macroAssembler_x86.cpp line 5737:
>
>> 5735: // The best case scenario is that there is no base or shift. Then
>> it is already
>> 5736: // a pointer that needs nothing but a register rename.
>> 5737: movptr(dst, src);
>
> Hm. So previous thing was zero-extending 32-bit value (e.g. narrow-oop). This
> one does 64-bit move, so it implicitly assumes higher 32-bits are zeroes. The
> existing comment is a bit misleading. Do we actually need this for AOT to
> work?
No, we don't need it for AOT code. Reverted. I changed it without thinking
about sign extension.
> src/hotspot/share/ci/ciEnv.cpp line 1124:
>
>> 1122: // No safepoints are allowed. Otherwise, class redefinition can
>> occur in between.
>> 1123: MutexLocker ml(Compile_lock);
>> 1124: NoSafepointVerifier nsv;
>
> OK, so here is an interesting lifecycle oddity. We are here arming `NSV` for
> class redefinition reasons. Looks fine until we go to
> `ciEnv::make_code_usable`, which builds `MCS` for preloaded methods. But
> `MCS` allocation is _Metaspace_ allocation, so it can trigger GC allocation
> failure and GC safepoint. So this `NSV` may fail.
In CompileBroker we generate MC unconditionally before creating compilation
task:
// Tiered policy requires MethodCounters to exist before adding a method to
// the queue. Create if we don't have them yet.
method->get_method_counters(thread);
Also in `ciEnv::is_compilation_valid()` we have:
// We require method counters to store some method state (max compilation
levels) required by the compilation policy.
if (method->get_method_counters(thread) == nullptr) {
record_failure("can't create method counters");
return false;
}
May be we should do the same check in CompileBroker.
> src/hotspot/share/code/aotCodeCache.cpp line 737:
>
>> 735:
>> 736: size_t codeCacheSize = pointer_delta(CodeCache::high_bound(),
>> CodeCache::low_bound(), 1);
>> 737: if (codeCacheSize > _codeCacheSize) { // Only allow smaller or equal
>> CodeCache size in production run
>
> It is not very clear to me why do we have this limit. What breaks if we have
> a larger code cache in production run? Some branches become not easily
> reachable, or something else? Let's polish the error message too: we need to
> say "larger" or "smaller" explicitly, so users can figure out what to adjust.
We will not be able to patch some branch/call instructions in AOT code if
CodeCache become bigger in production.
Remember that we have short branches and far branches. So distance to stubs,
for example, could change with bigger CodeCache and it would not fit into short
branch encoding.
> src/hotspot/share/code/aotCodeCache.cpp line 746:
>
>> 744: return false;
>> 745: }
>> 746: if ((_compressedKlassBase == nullptr ||
>> CompressedKlassPointers::base() == nullptr) && (_compressedKlassBase !=
>> CompressedKlassPointers::base())) {
>
> Wait, so this check only fires when _either_ of bases is `nullptr`? So if
> both bases are not null and disagree with each other, we just pass this
> check? Sounds like we want just the plain `==` here?
Encoding/Decoding code will be the same even if bases are different.
We only bailout when one base is NULL and the other is not.
> src/hotspot/share/code/aotCodeCache.hpp line 97:
>
>> 95: // Next field is exposed to external profilers - keep it as boolean.
>> 96: bool _for_preload; // Code can be used for preload
>> (before classes initialized)
>> 97: uint8_t _has_clinit_barriers:1, // Generated code has class init
>> checks (only in for_preload code)
>
> Same as nmethod bitfield, this yields a bitfield data race, which is straight
> up UB in C++.
>
> It would have been "fine" if field updates were under the same lock. But they
> are not: `set_not_entrant()` sets it under `NMethodState_lock`,
> `set_loaded()` sets another bit under `Compile_lock`, at very least. So break
> it up in separate fields?
Moved `_not_entrant` to separate field.
> src/hotspot/share/code/aotCodeCache.hpp line 494:
>
>> 492: // Here should be version and other verification fields
>> 493: enum {
>> 494: AOT_CODE_VERSION = 1
>
> Since we are extending the AOT code cache layout, it stands to reason we need
> to bump the version to catch incompatibilities? Although I suspect VM version
> fingerprinting already protects us from most of the problems.
You are right, I updated it. The next update will be when we rearrange AOT code
cache to separate code from metadata to mmap code directly into CodeCache.
> src/hotspot/share/code/nmethod.cpp line 1900:
>
>> 1898: const char* nm_kind = compile_kind();
>> 1899: if (nm_kind != nullptr) log->print(" compile_kind='%s'", nm_kind);
>> 1900: log->print(" compile_kind='%s'", nm_kind);
>
> There is already the printout for `compile_kind` one line above.
`compile_kind()` now returns "" instead of `nullptr`. I removed line with
condition.
> src/hotspot/share/code/nmethod.cpp line 1903:
>
>> 1901: log->print(" compiler='%s'", compiler_name());
>> 1902: if (TieredCompilation) {
>> 1903: log->print(" compile_level='%d'", comp_level());
>
> Do we really want to change XML output like this? Probably yields
> compatibility issues.
Reverted.
> src/hotspot/share/compiler/compiler_globals.hpp line 395:
>
>> 393: product(uint, DisableAOTCode, 0, DIAGNOSTIC,
>> \
>> 394: "Disable AOT code on some compilation levels "
>> \
>> 395: "(T1=1; T2=2; T4=4; T5/preload=8")
>> \
>
> Suggestion:
>
> "(T1=1; T2=2; T4=4; T5/preload=8)") \
May be I should follow #30352 changes and use decimal values: `(T1=1; T2=10;
T4=1000; T5/preload=10000)`
-------------
PR Comment: https://git.openjdk.org/jdk/pull/30778#issuecomment-4268341246
PR Comment: https://git.openjdk.org/jdk/pull/30778#issuecomment-4415751702
PR Comment: https://git.openjdk.org/jdk/pull/30778#issuecomment-4559447554
PR Review Comment: https://git.openjdk.org/jdk/pull/30778#discussion_r3210093577
PR Review Comment: https://git.openjdk.org/jdk/pull/30778#discussion_r3210245498
PR Review Comment: https://git.openjdk.org/jdk/pull/30778#discussion_r3210152552
PR Review Comment: https://git.openjdk.org/jdk/pull/30778#discussion_r3210132159
PR Review Comment: https://git.openjdk.org/jdk/pull/30778#discussion_r3210437106
PR Review Comment: https://git.openjdk.org/jdk/pull/30778#discussion_r3209980290
PR Review Comment: https://git.openjdk.org/jdk/pull/30778#discussion_r3209966923
PR Review Comment: https://git.openjdk.org/jdk/pull/30778#discussion_r3210330047
PR Review Comment: https://git.openjdk.org/jdk/pull/30778#discussion_r3210018193
