Revert initializeMethodRunAddressNoHook

[dsouzai]

initializeMethodRunAddressNoHook is called by JIT'd code when it needs
to revert to the interpeter. #20387 modified this function to set
the extra to 0x1. However, resetting the extra can result in hang
(see #20750 for details).

This commit reverts the change specific to
initializeMethodRunAddressNoHook.

Closes #20750
Fixes #20663

[dsouzai]

Fyi @ThanHenderson. @tajila could you please review?

[ThanHenderson]

For some context, I moved:

#if defined(J9VM_OPT_OPENJDK_METHODHANDLE)
	if (initializeMethodRunAddressMethodHandle(method)) {
		return;
	}
#endif /* defined(J9VM_OPT_OPENJDK_METHODHANDLE) */
#if defined(J9VM_OPT_METHOD_HANDLE)
	if (initializeMethodRunAddressVarHandle(method)) {
		return;
	}
#endif /* defined(J9VM_OPT_METHOD_HANDLE) */

from initializeMethodRunAddress to initializeMethodRunAddressNoHook to ensure that they would be run in the RAM class persistence work that is underway.

I also added method->extra = (void *) J9_STARTPC_NOT_TRANSLATED; before these moved calls to ensure that the input was the same as in initializeMethodRunAddress.

Taking another look at things, the code in initializeMethodRunAddressMethodHandle and initializeMethodRunAddressVarHandle does not depend on the value of method->extra. So, setting method->extra is not required here (as this change addresses).

However, should initializeMethodRunAddressVarHandle and initializeMethodRunAddressMethodHandle be run under all invocations of initializeMethodRunAddressNoHook (they weren't being run before for the use case that identified the hang). I don't want any more unexpected consequences to arise from this move, but to me, it seemed incorrect to have additional code being run in initializeMethodRunAddress that was not the code to run the hook, i.e.:

	if (J9_EVENT_IS_HOOKED(vm->hookInterface, J9HOOK_VM_INITIALIZE_SEND_TARGET)) {
		method->methodRunAddress = NULL;
		ALWAYS_TRIGGER_J9HOOK_VM_INITIALIZE_SEND_TARGET(vm->hookInterface, vmThread, method);
		if (NULL != method->methodRunAddress) {
			return;
		}
	}

@babsingh looking at the diffs, you added initializeMethodRunAddressMethodHandle, was there a reason this was added to initializeMethodRunAddress over initializeMethodRunAddressNoHook? (e.g. do these need to be run before ALWAYS_TRIGGER_J9HOOK_VM_INITIALIZE_SEND_TARGET(vm->hookInterface, vmThread, method);) If so, we should also revert that move, and I will add a different API for the snapshotting code to use.

[dsouzai]

However, should initializeMethodRunAddressVarHandle and initializeMethodRunAddressMethodHandle be run under all invocations of initializeMethodRunAddressNoHook (they weren't being run before for the use case that identified the hang).

Ah right, I was supposed to move both when opening this PR, but for some reason only remembered to remove the modification to the extra field. I think it's probably safest to move them back, but I'll wait for Babneet's response.

[babsingh]

We should move it back. The order matters due to the JIT dependency: J9JIT_REVERT_METHOD_TO_INTERPRETED.

[ThanHenderson]

The order matters due to the JIT dependency: J9JIT_REVERT_METHOD_TO_INTERPRETED.

Just to clarify, by "order matters" here, you aren't saying anything about initializeMethodRunAddressMethodHandle and initializeMethodRunAddressVarHandle needing to be run before the hook, you are talking about the call from the JIT not needing to run initializeMethodRunAddressMethodHandle and initializeMethodRunAddressVarHandle?

[babsingh]

initializeMethodRunAddressMethodHandle and initializeMethodRunAddressVarHandle needing to be run before the hook

Not completely sure about the dependency with the hook. The *VarHandle method preexisted before I introduced the *MethodHandle method. I ended up colocating the *MethodHandle method with the *VarHandle method to avoid issues.

[babsingh]

The hooks definition:

openj9/runtime/oti/j9vm.hdf

Lines 483 to 493 in c714a1e

	<name>J9HOOK_VM_INITIALIZE_SEND_TARGET</name>
	<description>
	Triggered when a RAM method is having its send target address initialized.
	The send target (methodRunAddress) of the method is NULL at the beginning of this event. If a listener sets methodRunAddress
	to another value, then further send target initialization is skipped, and the listener's value is used.
	</description>
	<trace-sampling intervals="101" />
	<struct>J9VMInitializeSendTargetEvent</struct>
	<data type="struct J9VMThread*" name="currentThread" description="current thread" />
	<data type="struct J9Method*" name="method" description="the method whose send target it about to be initialized" />
	</event>

[babsingh]

Both initializeMethodRunAddressMethodHandle and initializeMethodRunAddressVarHandle set methodRunAddress. As per the hook's definition, if the send target (methodRunAddress) is already set, the hook uses the value set by the listener.

J9HOOK_VM_INITIALIZE_SEND_TARGET (jitHookInitializeSendTarget) is a JIT hook. We need to check if this hook performs special optimizations based on the methodRunAddress set by initializeMethodRunAddressMethodHandle and initializeMethodRunAddressVarHandle. @dsouzai Would you know of any such operations?

[dsouzai]

J9HOOK_VM_INITIALIZE_SEND_TARGET (jitHookInitializeSendTarget) is a JIT hook. We need to check if this hook performs special optimizations based on the methodRunAddress set by initializeMethodRunAddressMethodHandle and initializeMethodRunAddressVarHandle. @dsouzai Would you know of any such operations?

I do see at least this in jitHookInitializeSendTarget:

method->methodRunAddress = jitGetCountingSendTarget(vmThread, method);

However, I'm not sure if there's other heuristics based on the value set. It might be that it's best we revert initializeMethodRunAddressNoHook and initializeMethodRunAddress back to what they were in case there are other effects that are not immediately obvious.

[ThanHenderson]

Wouldn't we be missing the correct binding (or at least go through a longer path) for the Method Handle send targets (e.g. J9_BCLOOP_SEND_TARGET_METHODHANDLE_INVOKEBASIC) if we don't have the call in the NoHook version? Instead, all those cases go to J9_BCLOOP_ENCODE_SEND_TARGET(J9_BCLOOP_SEND_TARGET_BIND_NATIVE); and resolve via.

[dsouzai]

Wouldn't we be missing the correct binding (or at least go through a longer path) for the Method Handle send targets (e.g. J9_BCLOOP_SEND_TARGET_METHODHANDLE_INVOKEBASIC) if we don't have the call in the NoHook version? Instead, all those cases go to J9_BCLOOP_ENCODE_SEND_TARGET(J9_BCLOOP_SEND_TARGET_BIND_NATIVE); and resolve via.

I don't know the answer to this; perhaps @tajila or @babsingh could comment on it.

[babsingh]

We reviewed the internal code review for the original change. Initially, initializeMethodRunAddressVarHandle was proposed within initializeMethodRunAddressNoHook. Following the review, it was moved to the start of initializeMethodRunAddress, before the hook, to ensure functional correctness. For now, the decision to revert to the original change is appropriate.

For other potential improvements, we will need to discuss the impact on VM-JIT interactions with @gacholio after their vacation next year.

[babsingh]

The commit message and PR description need to be updated to highlight the revert. Otherwise, the changes LGTM.

[babsingh]

jenkins test sanity zlinux jdk11,jdk21