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

[Timofei Pushkin]

On Mon, 28 Jul 2025 08:42:08 GMT, Radim Vansa <rvansa at openjdk.org> wrote:

>> 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
>
> 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?)

That was actually to cast-away `volatile` because it should not be needed under the lock. I am now rewriting this while fixing https://github.com/openjdk/crac/pull/251#discussion_r2235008362 so it will disappear.

> 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?

It can be done but `_klass_holder` field complicates things. As I see it, it will be either inefficient (fill `_klass_holder` in the temporary, then do it again in `_klass_holder` — an excess allocation of a weak handle) or ugly (don't initiate `_klass_holder` by default, add a method to initiate it, call it on the non-temporary — until this method is called the object is in a weird semi-initialized state). I would prefer to just leave it as is.

It is sad that `GrowableArray` and HotSpot in general do not support move semantics...

> 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.

There are two properties now:
- `jdk.crac.enable-recompilation` — setting it to false enables the old behavior, it is static final because letting it change at runtime would complicate things and I don't think it is so much needed
- `jdk.crac.recompilation-delay-ms` — same as before but can be changed at runtime, a warning is printed if it is set when `jdk.crac.enable-recompilation` is false

> 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...

I am not sure what kind of false positives can there be, as I see it, in worst case we will just wait longer then necessary.

WhiteBox API indeed does not provide a way to wait for a compilation. I could probably indeed make the parent process read `PrintCompilation` from stdout and print to stdin but I am not sure this is needed as it is more complicated.

> 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?

I don't think this particular assert can be removed even if we would read logs.

It asserts that the machine had time to deoptimize the method before the delay expired. Because if we do it after the delay expires it is expected that there will be no recompilation. Reading logs won't free us from this assert.

The only thing I can come up with is to implement some mechanism to relaunch the test (or just the restore part if I implement the request above to make recompilation delay restore-settable) with a higher delay when the test fails this way.

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

PR Review Comment: https://git.openjdk.org/crac/pull/251#discussion_r2236770641
PR Review Comment: https://git.openjdk.org/crac/pull/251#discussion_r2237407052
PR Review Comment: https://git.openjdk.org/crac/pull/251#discussion_r2237388627
PR Review Comment: https://git.openjdk.org/crac/pull/251#discussion_r2236845446
PR Review Comment: https://git.openjdk.org/crac/pull/251#discussion_r2237161288