[crac] RFR: 8364156: [CRaC] Recompile methods decompiled during or shortly after C/R [v2]

Tue Jul 29 06:43:20 UTC 2025

On Tue, 29 Jul 2025 06:39:09 GMT, Timofei Pushkin <tpushkin at openjdk.org> wrote:

>> Implements recording and triggering recompilation of methods decompiled during and shortly after C/R. The length of the "shortly-after" period can be controlled by the user via `-Djdk.crac.recompilation-delay-ms=<long>`, the default is 10ms.
>> 
>> Measuring the after-restore regression before and after this change shows improvement on hello-world apps. The numbers are the regression of the first after-restore 1 sec iteration vs average over 5 last before-checkpoint 1 sec iterations, i.e. lower means better; mean ± std dev over 30 runs. Measured in a Linux VM on MacOS, with CRIU, with 5ms after-restore recompilation delay.
>> 
>> |                          | Before      | After       |
>> |--------------------------|-------------|-------------|
>> | Helidon hello-world      | 0.9 ± 1.2 % | 0.4 ± 1.0 % |
>> | Spring boot CRaC example | 9.5 ± 2.4 % | 6.4 ± 2.5 % |
>> 
>> Currently decompilations are recorded during C/R + during the after-restore delay and then they all get recompiled together. An enhancement planned in the future is to record only during C/R recompiling immediately after restore and during the after-restore delay recompile immediately without recording. This way the recompilations would finish faster, also setting `jdk.crac.recompilation-delay-ms` to a higher value would have less negative impact.
>> 
>> Implementation notes:
>> - Native code (`nmethod`) is considered "decompiled" when `make_not_entrant(...)` is called on it — a new mandatory parameter is added to the signature of this method that controls whether CRaC should consider this particular decompilation for recompilation or not.
>>   - The parameter is needed because we do not want to recompile, for example, when `nmethod` is thrown away once a replacement for it has been compiled (e.g. on another compilation level).
>>   - The parameter has no default so that we get compilation errors when new calls to `make_not_entrant(...)` are introduced in the mainline — we will be deciding whether these new calls should be considered for recompilation or not. If CRaC was in the mainline I would enable the recompilation by default.
>> - The new native `CRaCRecorder` class is the core of the implementation — it records the decompilations and handles their recompilation.
>>   - `jdk.crac.internal.mirror.Core` notifies it when to start and stop the recording, it also handles the after-restore recompilation delay.
>>   - `nmethod.make_not_entrant(...)` notifies it about decompiled code.
>
> Timofei Pushkin has updated the pull request incrementally with five additional commits since the last revision:
> 
>  - Implement flag-related review suggestions
>  - Cleanup tests
>  - Remove erroneously added assert
>  - Bring back erroneously removed locking
>  - Fix RedefineClasses defence

Great work! We shall give some thought to the possible timing issues in the tests, as timing-related failures in the testsuite are really annoying.

src/hotspot/share/runtime/cracRecompiler.cpp line 151:

> 149:   }
> 150:   const MutexLocker ml(decompilations_lock, Mutex::_no_safepoint_check_flag);
> 151:   auto * const decomps = const_cast<GrowableArrayCHeap<CompilationInfo *, MemTag::mtInternal> *>(decompilations);

Do you really need the `const_cast` here? I wonder why you use `GrowableArrayCHeap<...> * const` here and `const GrowableArrayCHeap<...> *` in the next function (or am I misinterpreting the `auto` use?)

src/hotspot/share/runtime/cracRecompiler.cpp line 198:

> 196:     assert(decompilations != nullptr, "unexpected state: is_recording = %s, decompilations = %p",
> 197:            BOOL_TO_STR(is_recording), decompilations);
> 198:     decompilations->append(new CompilationInfo(nmethod.method(),

Would it be problematic to make `CompilationInfo` copy-able, rather than allocating each element individually?

src/java.base/share/classes/jdk/internal/crac/mirror/Core.java line 70:

> 68:         public static final boolean TRACE_STARTUP_TIME =
> 69:             Boolean.getBoolean("jdk.crac.trace-startup-time");
> 70:         public static final long RECOMPILATION_DELAY_MS =

I am thinking about an option for the 'old behaviour', at least for some testing. Could you interpret negative values as "don't disable the recompilations at all"?

And another thing: by putting this into a static variable you're fixing the value at checkpoint time. I believe it would be better if we could adjust this on restore (I think that at the point where you use this the system properties are already updated). Naturally if the recompilations were not disabled through a negative value this should have not effect but a warning being printed out.

test/jdk/jdk/crac/recompiler/NaturalDecompilationTest.java line 120:

> 118:             }
> 119:             try {
> 120:                 Thread.sleep(500); // Time to compile

I think this has some potential for false positives in the testsuite. I guess there is no API in whitebox to wait until this is observed? Alternatively watching the output in `test()` and signaling the process somehow...

test/jdk/jdk/crac/recompiler/RecompilationDelayTest.java line 96:

> 94: 
> 95:             @Override
> 96:             public void beforeCheckpoint(Context<? extends Resource> context) throws Exception {}

total nitpick: you can remove the `throws Exception`...

test/jdk/jdk/crac/recompiler/RecompilationDelayTest.java line 115:

> 113: 
> 114:         if (delayMs > 0) {
> 115:             assertEquals(

10 seconds sounds like enough, but ideally I would have tests as independent to timing as possible. Are there any timestamps in the compilation log so rather than checking the current time we could just read the logs and run assertions on those?

-------------

PR Review: https://git.openjdk.org/crac/pull/251#pullrequestreview-3061259649
PR Review Comment: https://git.openjdk.org/crac/pull/251#discussion_r2235358504
PR Review Comment: https://git.openjdk.org/crac/pull/251#discussion_r2237085852
PR Review Comment: https://git.openjdk.org/crac/pull/251#discussion_r2235606890
PR Review Comment: https://git.openjdk.org/crac/pull/251#discussion_r2235585317
PR Review Comment: https://git.openjdk.org/crac/pull/251#discussion_r2235626356
PR Review Comment: https://git.openjdk.org/crac/pull/251#discussion_r2235662916