RFR: 8316694: Implement relocation of nmethod within CodeCache [v19]

[Chad Rakoczy]

On Tue, 3 Jun 2025 15:49:30 GMT, Tom Rodriguez <never at openjdk.org> wrote:

>>> So this copying keeps the same compile_id, which sort of makes sense but it's also potentially confusing. What's the plan for how this interacts with flags like PrintNMethods and JVMTI code installation notification? This is done in nmethod::post_compiled_method which doesn't seem to be used on the new nmethod. If the reclamation of the old nmethod is performed in the normal fashion, we now have 2 nmethods alive with the same compile_id which could be confusing. But allocating a new compile_id breaks the connection to the original compile which seems bad too.
>> 
>> As we are not compiling, `compile_id` should stay the same. Yes, we need to add some logging: `log_info(codecache)` and `PrintNMethods`. 
>> 
>> According to https://docs.oracle.com/en/java/javase/24/docs/specs/jvmti.html#CompiledMethodLoad, compile methods can be moved. We need to generate events if it happens:
>>>If it is moved, the [CompiledMethodUnload](https://docs.oracle.com/en/java/javase/24/docs/specs/jvmti.html#CompiledMethodUnload) event is sent, followed by a new CompiledMethodLoad event.
>> 
>>  > we now have 2 nmethods alive with the same compile_id which could be confusing.
>> 
>> If `compile_id` is interpreted as id of nmethod, it is confusing. Comment to `nmethod::_compile_id`: https://github.com/openjdk/jdk/blob/aea2837143289800cfbb7044de4f105e87e233ff/src/hotspot/share/code/nmethod.hpp#L259
>> 
>> According to it, it is id of a compilation task. In such case there should be no confusion.
>
>> If it is moved, the [CompiledMethodUnload](https://docs.oracle.com/en/java/javase/24/docs/specs/jvmti.html#CompiledMethodUnload) event is sent, followed by a new CompiledMethodLoad event.
> 
>>    we now have 2 nmethods alive with the same compile_id which could be confusing.
> 
> It's nice that the JVMTI docs considered this problem but the notifications will be sent in the reverse order given our current implementation.  We will create a new nmethod while the old nmethod might still be alive, at least for the purposes of deopt.  Since this PR doesn't actually perform any relocation, I'm not sure what the plan is here.  The most aggressive thing that could be done is to invalidate all frames which have the old nmethod on stack, but that still leaves the nmethod live for the purposes of deopt.  It would probably be ok to synthesize an unload after the deopt since there should be no actual execution in those nmethods, but you will then have to suppress the one that's normally done during nmethod::unlink.
> 
> I agree that the docs are fairly clear that all of this is ok, but that doesn't mean that assumptions haven't been made about the current implementation.  We just need to make sure we do something rational and that it's possible to understand from our output what was done.

@tkrodriguez

> It's nice that the JVMTI docs considered this problem but the notifications will be sent in the reverse order given our current implementation. We will create a new nmethod while the old nmethod might still be alive, at least for the purposes of deopt. Since this PR doesn't actually perform any relocation, I'm not sure what the plan is here. 

I actually think it is better if we do not send unload events during relocation and allow the current code path (`nmethod::unlink()`) to send the unload event. Like you mentioned "relocation" isn’t actually moving the nmethod as it just copies and invalidates the original. According to the JVMTI spec:
>> Note that a single method may have multiple compiled forms, and that this [CompiledMethodLoad] event will be sent for each form

So it should be safe for us to do this. 

> The most aggressive thing that could be done is to invalidate all frames which have the old nmethod on stack, but that still leaves the nmethod live for the purposes of deopt. It would probably be ok to synthesize an unload after the deopt since there should be no actual execution in those nmethods, but you will then have to suppress the one that's normally done during nmethod::unlink.

With the updated approach I don’t think this is necessary. Since the original nmethod is only being marked as not entrant it is still safe for it to be “live”. This allows us to avoid special cases for relocated nmethods and it should behave similarly to as if it were recompiled.

I also added a test to verify that we correctly publish the load and unload events during relocation ([source](https://github.com/openjdk/jdk/pull/23573/commits/b3358bda645c68f1ffccdfdcb98c44ee0ec69ce0))

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

PR Comment: https://git.openjdk.org/jdk/pull/23573#issuecomment-2989283417