RFR: 8316694: Implement relocation of nmethod within CodeCache

[Chad Rakoczy]

On Thu, 20 Feb 2025 22:05:34 GMT, Evgeny Astigeevich <eastigeevich at openjdk.org> wrote:

>> This PR introduces a new function to replace nmethods, addressing [JDK-8316694](https://bugs.openjdk.org/browse/JDK-8316694). It enables the creation of new nmethods from existing ones, allowing method relocation in the code heap and supporting [JDK-8328186](https://bugs.openjdk.org/browse/JDK-8328186).
>> 
>> When an nmethod is replaced, a deep copy is performed. The corresponding Java method is updated to reference the new nmethod, while the old one is marked as unused. The garbage collector handles final cleanup and deallocation.
>> 
>> This change does not modify existing code paths and therefore does not benefit much from existing tests. New tests were created and confirmed to pass on x64/aarch64 for slowdebug/fastdebug/release.
>
> src/hotspot/cpu/aarch64/relocInfo_aarch64.cpp line 93:
> 
>> 91: void trampoline_stub_Relocation::pd_fix_owner_after_move() {
>> 92:   NativeCall* call = nativeCall_at(owner());
>> 93:   // assert(call->raw_destination() == owner(), "destination should be empty");
> 
> We need to move this assert to `trampoline_stub_Relocation::fix_relocation_after_move`.
> `CodeBuffer::blob()` returns `nullptr` if it wraps `nmethod`.
> The modified assert will be:
> 
> void trampoline_stub_Relocation::fix_relocation_after_move(const CodeBuffer* src, CodeBuffer* dest) {
>   // Finalize owner destination only for nmethods
>   if (dest->blob() != nullptr) return;
>   // We either relocate a nmethod residing in CodeCache or just generated code from CodeBuffer
>   assert(src->blob() != nullptr || nativeCall_at(owner())->raw_destination() == owner(), "destination should be empty");
>   pd_fix_owner_after_move();
> }

Shouldn't the check be `src->blob() == nullptr` so the assert passes if relocating an `nmethod`?

> src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp line 1382:
> 
>> 1380: 
>> 1381:     // First instruction must be a nop as it may need to be patched after relocation
>> 1382:     __ nop();
> 
> Could you please explain what a problem it fixes?

When an `nmethod` gets marked as not entrant the [first instruction is updated](https://github.com/openjdk/jdk/blob/11a37c829c12d064874416a7b242596cf23972e5/src/hotspot/cpu/aarch64/nativeInst_aarch64.cpp#L368) to verify that the code is no longer called. The first instruction must be a jump or nop for this

> src/hotspot/share/code/nmethod.cpp line 1517:
> 
>> 1515:     _method->clear_entry_points();
>> 1516:     _method->set_code(mh, this);
>> 1517:   }
> 
> Why do we need this code? Why not to do the same when when we replace C1 nmethod with C2 nmethod?

`set_code` has an [assert](https://github.com/openjdk/jdk/blob/11a37c829c12d064874416a7b242596cf23972e5/src/hotspot/share/oops/method.cpp#L1298) that the entry is not already set for continuation native intrinsics. It's normally not an issue because it's an intrinsic and never gets recompiled. It's probably better to remove or update that assert instead of calling this for every relocation

> src/hotspot/share/code/nmethod.cpp line 1566:
> 
>> 1564:   nm->make_deoptimized();
>> 1565:   nm->flush_dependencies();
>> 1566:   nm->set_is_unlinked();
> 
> Why are you explicitly making these calls?

Those are actually not needed and will be removed in the next revision

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23573#discussion_r1970713869
PR Review Comment: https://git.openjdk.org/jdk/pull/23573#discussion_r1982342344
PR Review Comment: https://git.openjdk.org/jdk/pull/23573#discussion_r1982339643
PR Review Comment: https://git.openjdk.org/jdk/pull/23573#discussion_r1974557724