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

Thu May 8 20:28:58 UTC 2025

On Fri, 25 Apr 2025 22:31:28 GMT, Erik Österlund <eosterlund at openjdk.org> wrote:

>>> Hi @fisk,
>>> 
>>> Thank you for the very valuable comment. It has point we have not thought about.
>>> 
>>> > I am not fond of "special nmethods" that work subtly different to normal nmethods and have their own special life cycles.
>>> 
>>> It's not clear to me what you mean "special nmethods". IMO we don't introduce any special nmethods. From my point of view, a normal nmethod is an nmethod for a ordinary Java method. Nmethods for non-ordinary Java methods are special, e.g. native nmethods or method handle linkers(JDK-8263377). I think normal nmethods should be relocatable within CodeCache.
>> 
>> I mean nmethods with a subtly different life cycle where usual invariants/expectations don't hold. Like method handle intrinsics and enter special intrinsics for example. Used to have a different life cycle for OSR nmethods too.
>> 
>>> > You can't just copy oops.
>>> 
>>> Yes, this is the main issue at the moment. Can we do this at a safepoint?
>> 
>> I don't think it solves much. You can't stash away a pointer to the nmethod, roll to a safepoint, and expect the nmethod to not be freed. Even if you did, you still can't copy the oops.
>> 
>> If we are to do this, I think you want to apply nmethod entry barriers first. That stabilizes the oops.
>> 
>>> > I'm worried about copying the nmethod epoch counters
>>> 
>>> We should clear them. If not, it is a bug.
>> 
>> I'd like to change copying from opt-out to opt-in instead; that would make me feel more comfortable. Then perhaps you can share initialization code that sets up the initial state of the nmethod exactly in the same way as normal nmethods.
>> 
>> I didn't check but you need to take the Compile_lock and verify dependencies too if you didn't do that, I think, so you don't race with deoptimization.
>> 
>>> > You don't check if the nmethod is_unloading() when cloning it.
>>> 
>>> Should such nmethods be not entrant? We don't relocate not entrant nmethods.
>> 
>> is_not_entrant doesn't imply is_unloading.
>> 
>>> > What are the consequences of copying the deoptimization generation?
>>> 
>>> What do you mean?
>> 
>> I mean is it safe to racingly copy the deoptmization generation when there is concurrent deoptimization? This is why I'd prefer copying to be opt-in rather than opt-out so we don't have to stare at every single field and wonder what will happen when a new nmethod "inherits" state from a different nmethod in interesting races. I want it to work as much as possible as normal nmethod installation, starting with a state as close as po...
>
>> @fisk Thank you for the valuable feedback. Here is a more detailed response to the concerns you brought up
> 
> Thanks, it's shaping up.
> 
>> Instead of tracking the nmethod pointer which could become stale I updated the code to use method handles. I believe the method handle should ensure the method remains valid and we can then relocate its corresponding nmethod. [Reference](https://github.com/chadrako/jdk/blob/027f5245a6829e79bd8624c1cca542c4c24ace5c/src/hotspot/share/runtime/vmOperations.cpp#L106-L110)
> 
> The safepoint is still causing more trouble than it solves. It was introduced due to oop phobia. What the oops really needed to stabilize is to run the entry barrier which you do now. The safepoint merely destabilizes the oops again while introducing latency problems and fun class redefinition interactions. It should be removed as I can't see it serves any purpose.
> 
>> The relocated nmethod is added as a dependent nmethod on all of the MethodHandles and InstranceKlass in its dependency scope. [Reference](https://github.com/chadrako/jdk/blob/027f5245a6829e79bd8624c1cca542c4c24ace5c/src/hotspot/share/code/nmethod.cpp#L1543-L1564)
> 
> My concern was about something else - a table tracks all the nmethods that have old metadata in order to speed up a walk over the code cache that finds said nmethods.
> 
> This should be dealt with by not relocating nmethods with evol dependencies/metadata and by not safepointing, which could introduce class redefinition which populates this table.
> 
>> The source nmethod entry barrier is now called before copying. I believe this will disarm the barrier and reset the guard value for it to be safe to copy. [Reference](https://github.com/chadrako/jdk/blob/027f5245a6829e79bd8624c1cca542c4c24ace5c/src/hotspot/share/code/nmethod.cpp#L1530)
> 
> Yes and fix the oops so you don't need a safepoint.
> 
>> Copying this value was not intentional. It should be correctly set to the default value now. [Reference](https://github.com/chadrako/jdk/blob/027f5245a6829e79bd8624c1cca542c4c24ace5c/src/hotspot/share/code/nmethod.cpp#L1441)
> 
> Good.
> 
>> I added this check to ensure the nmethod is not unloading and removed the not entrant check as is unloading implies not entrant. [Reference](https://github.com/chadrako/jdk/blob/027f5245a6829e79bd8624c1cca542c4c24ace5c/src/hotspot/share/code/nmethod.cpp#L1583-L1585)
> 
> That's not quite true. There are two separate mechanisms that guard the entry. When sn nmethod becomes invalid due to for example a broken speculative assumpti...

@fisk I believe I have addressed the remaining issues you brought up

> The safepoint is still causing more trouble than it solves. It was introduced due to oop phobia. What the oops really needed to stabilize is to run the entry barrier which you do now. The safepoint merely destabilizes the oops again while introducing latency problems and fun class redefinition interactions. It should be removed as I can't see it serves any purpose.

Relocation no longer occurs at a safepoint

> My concern was about something else - a table tracks all the nmethods that have old metadata in order to speed up a walk over the code cache that finds said nmethods.
> This should be dealt with by not relocating nmethods with evol dependencies/metadata and by not safepointing, which could introduce class redefinition which populates this table.

I added the check for evol dependecies/metadata to not relocate them ([reference](https://github.com/chadrako/jdk/blob/9ca3563a0fe8e021a7a99107a4b675d2210a34b2/src/hotspot/share/code/nmethod.cpp#L1616))

> Okay. Speaking of which, seems like the NMethodState_lock is held for way too long - usually just held when setting the Method code and updating the nmethod state after the initial state is set. Keeping the lock across other things makes me worried of deadlocks.

The NMethodState_lock is now only held when the state gets updated ([reference](https://github.com/chadrako/jdk/blob/9ca3563a0fe8e021a7a99107a4b675d2210a34b2/src/hotspot/share/code/nmethod.cpp#L1575))

> Have you checked what the JVMCI speculation data and JVMCI data contains and if your approach will break that? JVMCI has an nmethod mirror object that refers back to the nmethod - this is unlikely to work out of the box with cloning.

The HotspotNMethod represented by the nmethod mirror is now updated to reflect the new nmethod ([reference](https://github.com/chadrako/jdk/blob/9ca3563a0fe8e021a7a99107a4b675d2210a34b2/src/hotspot/share/code/nmethod.cpp#L1582-L1583) [reference](https://github.com/chadrako/jdk/blob/9ca3563a0fe8e021a7a99107a4b675d2210a34b2/src/hotspot/share/jvmci/jvmciRuntime.cpp#L851-L863))

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

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