Abort compilations for exceptions during invokedynamic resolution #17625

nbhuiyan · 2023-06-20T19:59:46Z

Invokedynamic call resolution can either result in a valid entry
in the CallSite table if successful, or an exception object
otherwise. The call site table entry is a two element array
containing the MemberName and appendix objects necessary for
constructing a resolved invokedynamic call. The exception object
would not be an array type object, and therefore it is sufficient
to check if the entry obtained from the CallSite table is an array
instance to determine if the resolution was successful.

This changeset handles the cases when we have an invalid entry
in the CallSite table that results in either failed inlining or
failed compilation, depending on whether the invokedynamic is part
of the inlining candidate or the root method, respectively.

Issue: #16965

nbhuiyan · 2023-06-20T20:00:05Z

@0xdaryl Requesting review

nbhuiyan · 2023-06-20T22:02:55Z

I believe the new VM API being added will need a JITServer override. I will try to implement that as well.

0xdaryl · 2023-06-20T20:12:24Z

runtime/compiler/env/VMJ9.h

@@ -279,6 +279,16 @@ class TR_J9VMBase : public TR_FrontEnd
   virtual bool jitStaticsAreSame(TR_ResolvedMethod *, int32_t, TR_ResolvedMethod *, int32_t);
   virtual bool jitFieldsAreSame(TR_ResolvedMethod *, int32_t, TR_ResolvedMethod *, int32_t, int32_t);

+   /**
+    * \brief Check is object is an array class instance


First "is" -> "if"

0xdaryl · 2023-06-20T20:13:45Z

runtime/compiler/env/VMJ9.h

+    *
+    * \param currentThread the current thread
+    * \param object the object address
+    * \return true if object is an array class instance


You can just say ; false otherwise for brevity and delete the next line.

0xdaryl · 2023-06-20T23:10:25Z

runtime/compiler/env/VMJ9.h

+    * \return true if object is an array class instance
+    * \return false if object is not an array class instance
+    */
+   virtual bool objectIsArray(J9VMThread *currentThread, TR_OpaqueClassBlock *object);


You may have chosen this name because it matches the helper you end up calling, but since this is an inquiry API in the FrontEnd would it be better to call it isArrayObject() instead?

0xdaryl · 2023-06-21T17:27:45Z

Just confirming.... Is this issue only with the JIT's (mis)understanding of invokedynamic and the fix applies to both the OpenJ9 MH implementation as well as the OpenJDK LambdaForm MH implementations?

nbhuiyan · 2023-06-21T18:41:21Z

@0xdaryl

Is this issue only with the JIT's (mis)understanding of invokedynamic and the fix applies to both the OpenJ9 MH implementation as well as the OpenJDK LambdaForm MH implementations?

No, this fix only applies to the OpenJDK MH implementation. The older OpenJ9 MH implementation has a different mechanism for invokedynamic resolution, and does not place an exception object in the CallSite table entry, and the valid table entries themselves are not arrays.

nbhuiyan · 2023-06-21T18:42:58Z

@0xdaryl I have addressed your initial review comments. There are some JITServer test failures due to the missing JITServer implementation. I am currently working on that.

0xdaryl · 2023-06-21T19:48:24Z

No, this fix only applies to the OpenJDK MH implementation. The older OpenJ9 MH implementation has a different mechanism for invokedynamic resolution, and does not place an exception object in the CallSite table entry, and the valid table entries themselves are not arrays.

OK, I just expanded the code around the fix and saw the #ifdefs guarding it, which is what I was going to ask you to add. So it is all good.

Tag me when you think this PR is ready for another review.

nbhuiyan · 2023-06-21T23:07:58Z

The JITServer test failures I observed before in my personal build now appear to be passing. @0xdaryl Ready for review.

@mpirvu I'd appreciate your review of the JITServer changes

0xdaryl · 2023-06-22T16:17:41Z

Jenkins test sanity,sanity.openjdk all jdk17

0xdaryl · 2023-06-22T16:18:09Z

Jenkins test sanity,sanity.openjdk all jdk21

mpirvu

LGTM
Since a new message has been added, could you please increment the MINOR_NUMBER here
https://github.com/eclipse-openj9/openj9/blob/master/runtime/compiler/net/CommunicationStream.hpp#L121

Also, I was wondering why the parameter for isArrayObject(TR_OpaqueClassBlock *object); is declared to be as a pointer to a class, when in fact it's an object.

nbhuiyan · 2023-06-22T20:49:25Z

@mpirvu I have updated the MINOR_NUMBER

I was wondering why the parameter for isArrayObject(TR_OpaqueClassBlock *object); is declared to be as a pointer to a class, when in fact it's an object

Thanks for pointing that out, I should not be using TR_OpaqueClassBlock* type for an object. Fixed that as well.

mpirvu

I am having second thoughts about the implementation. The code seems to be peeking inside objects without making sure it has VM access. Without VM access in hand, the GC can move the objects while the compiler attempts to inspect them. I made some suggestions on how the code needs to be changed.
Moreover, the JITServer should never receive any object references. Thus, the new frontend method should just fatal assert if executed at the server rather than sending a message at the client.

mpirvu · 2023-06-22T23:02:43Z

runtime/compiler/env/j9method.cpp

+      // and appendix objects necessary for constructing a resolved invokedynamic adapter method call.
+      // The exception object would not be an array type object, and therefore it is sufficient to check if
+      // the entry obtained from the CallSite table is an array instance to determine if the resolution was successful.
+      if (!fej9()->isArrayObject(*invokeCacheArray))


I am having second thoughts about this one. if *invokeCacheArray is truly a pointer to an object, what prevents GC from moving that object while we try to access it? We may need to hold VMAccess while executing if (!fej9()->isArrayObject(*invokeCacheArray)) (extend the VMAccessCriticalSection which starts below).
There is also a comment below that "// this will not work in AOT or JITServer", so probably there is some guarantee that we cannot enter this path for JITServer.

if *invokeCacheArray is truly a pointer to an object, what prevents GC from moving that object while we try to access it?

I did not feel it was necessary to ensure VM access for this. My reasoning was, immediately after checking if the corresponding entry in the call site table had a valid object pointer using

openj9/runtime/compiler/env/j9method.cpp

Lines 5915 to 5919 in ab5dfcf

bool

TR_ResolvedJ9Method::isUnresolvedCallSiteTableEntry(int32_t callSiteIndex)

{

return *(j9object_t*)callSiteTableEntryAddress(callSiteIndex) == NULL;

}

We want to check whether that non-null object pointer cached in the call site table slot was an array or not. If that object pointer at ramClass->callSites[callSiteIndex] (i.e, *invokeCacheArray) is not pointing to a valid array object, then we should abort anyway. The slot is populated by an object that the VM's invokedynamic resolution mechanism allocates and (I'm speculating) the GC probably can not move. It's not a regular Java object. Once a call site table slot is populated (only if it was NULL to begin with), it is not modified again. What we do need VM access for however is accessing the individual elements in the invokeCacheArray, which is why we have the VM access being acquired right after this.

Based on my reasoning, do you still think it is necessary to put the isArrayObject check within the block with VM access acquired?

There is also a comment below that "// this will not work in AOT or JITServer", so probably there is some guarantee that we cannot enter this path for JITServer.

This was true when this code was first written during the initial OpenJDK MethodHandle implementation, but since then JITServer became capable of supporting OpenJDK MHs and I can confirm that this code path is executed in JITServer now based on my testing. I will update the comment here to reflect the current state.

Responding to your overall review comment:

Moreover, the JITServer should never receive any object references. Thus, the new frontend method should just fatal assert if executed at the server rather than sending a message at the client.

Wouldn't this mean that JITServer loses support for any method containing invokedynamic bytecodes?

If that array stored in one of the callsites is really special and cannot be moved by GC (maybe because it does not reside in the heap), then you don't need VM access for checking whether it's an array. Interesting though that the two elements of the array are protected by VM access.

fej9()->targetMethodFromMemberName() has a FATAL_ASSERT in the JITServer implementation.

TR_OpaqueMethodBlock* TR_J9ServerVM::targetMethodFromMemberName(uintptr_t memberName) { TR_ASSERT_FATAL(false, "targetMethodFromMemberName must not be called on JITServer"); return NULL; }

so if we ever entered that path while compiling at the server, the assert would be triggered.

Another point: getResolvedDynamicMethod() has a different implementation for JITServer where the server asks the client to execute the corresponding method and then, with the answers received from the client, the server builds a resolvedMethod. Thus, the server does not need to access Java objects.

Thank you for pointing those out, I had the wrong understanding of how things were being handled in JITServer mode. I will make the changes as you suggested.

I will also enforce VM access requirement for checking if object is array, so as not to cause confusion in the future for others, as there is no real benefit to doing this without VM access but lots of potential danger if this API was later used to check if heap objects were array types.

runtime/compiler/optimizer/InterpreterEmulator.cpp

mpirvu · 2023-06-22T23:21:53Z

I am surprised though that we cache object references in J9Class (J9Class->callSites[]). Either GC cannot move such objects or somehow it updates all these pointers when objects are moved.

nbhuiyan · 2023-07-06T20:44:37Z

@mpirvu I have updated the implementation based on the review comments. I would appreciate another review.

runtime/compiler/control/JITClientCompilationThread.cpp

runtime/compiler/net/MessageTypes.cpp

runtime/compiler/net/MessageTypes.hpp

runtime/compiler/env/VMJ9.h

runtime/compiler/env/VMJ9Server.cpp

mpirvu

LGTM

mpirvu · 2023-07-10T19:10:59Z

jenkins test sanity all jdk20

mpirvu · 2023-07-11T10:18:19Z

jenkins test sanity plinuxjit,xlinuxjit,zlinuxjit jdk20

mpirvu · 2023-07-11T17:28:49Z

All tests have passed.
@0xdaryl if you officially approve this PR I can merge it. Thanks

runtime/compiler/env/j9method.cpp

Invokedynamic call resolution can either result in a valid entry in the CallSite table if successful, or an exception object otherwise. The call site table entry is a two element array containing the MemberName and appendix objects necessary for constructing a resolved invokedynamic call. The exception object would not be an array type object, and therefore it is sufficient to check if the entry obtained from the CallSite table is an array instance to determine if the resolution was successful. This changeset handles the cases when we have an invalid entry in the CallSite table that results in either failed inlining or failed compilation, depending on whether the invokedynamic is part of the inlining candidate or the root method, respectively. Signed-off-by: Nazim Bhuiyan <[email protected]>

isInvokeCacheEntryAnArray is used to check if invokedynamic bytecodes' CallSite table entries are valid invoke cache array objects. If the resolution fails, we end up with an exception object in the corresponding table slot instead of an array object. Signed-off-by: Nazim Bhuiyan <[email protected]>

Signed-off-by: Nazim Bhuiyan <[email protected]>

nbhuiyan · 2023-07-12T02:01:43Z

@0xdaryl I have addressed your comments. The latest force push addressed indentation issues, removed an empty line I accidentally added, and added curly braces for an if statement. As the tests have passed earlier although not visible due to the updated branch, relaunching them should not be necessary for these changes alone.

0xdaryl · 2023-07-12T17:26:41Z

Recent changes were mostly whitespace. Previous testing was successful. No need to re-run CI. Merging.

babsingh mentioned this pull request Jun 20, 2023

JDK20 jdk/incubator/concurrent/ScopedValue/StressStackOverflow.java StressStackOverflow$TestFailureException: Unexpected value for ScopedValue #16965

Open

0xdaryl reviewed Jun 20, 2023

View reviewed changes

0xdaryl self-assigned this Jun 21, 2023

0xdaryl added the comp:jit label Jun 21, 2023

nbhuiyan force-pushed the invokedynamic-exception branch from 2a8225f to 058b1e0 Compare June 21, 2023 16:35

nbhuiyan changed the title ~~Handle cases of failed invokedynamic resolution~~ Abort compilations for exceptions during invokedynamic resolution Jun 21, 2023

nbhuiyan force-pushed the invokedynamic-exception branch from 058b1e0 to 9da1856 Compare June 21, 2023 18:33

nbhuiyan changed the title ~~Abort compilations for exceptions during invokedynamic resolution~~ WIP: Abort compilations for exceptions during invokedynamic resolution Jun 21, 2023

nbhuiyan changed the title ~~WIP: Abort compilations for exceptions during invokedynamic resolution~~ Abort compilations for exceptions during invokedynamic resolution Jun 21, 2023

nbhuiyan force-pushed the invokedynamic-exception branch from 3426a6d to 84a38de Compare June 21, 2023 23:06

mpirvu reviewed Jun 22, 2023

View reviewed changes

nbhuiyan changed the title ~~Abort compilations for exceptions during invokedynamic resolution~~ WIP: Abort compilations for exceptions during invokedynamic resolution Jun 23, 2023

nbhuiyan force-pushed the invokedynamic-exception branch from 48470a6 to 6866cb6 Compare July 6, 2023 20:43

nbhuiyan changed the title ~~WIP: Abort compilations for exceptions during invokedynamic resolution~~ Abort compilations for exceptions during invokedynamic resolution Jul 6, 2023

nbhuiyan force-pushed the invokedynamic-exception branch from 6866cb6 to 15fcf3e Compare July 6, 2023 20:48

mpirvu reviewed Jul 8, 2023

View reviewed changes

nbhuiyan force-pushed the invokedynamic-exception branch from 15fcf3e to b314536 Compare July 9, 2023 00:39

mpirvu approved these changes Jul 10, 2023

View reviewed changes

0xdaryl reviewed Jul 11, 2023

View reviewed changes

runtime/compiler/env/j9method.cpp Outdated Show resolved Hide resolved

runtime/compiler/env/j9method.cpp Outdated Show resolved Hide resolved

nbhuiyan added 3 commits July 11, 2023 21:54

Increment JITServer MINOR_NUMBER for VM_isInvokeCacheEntryAnArray

c67008b

Signed-off-by: Nazim Bhuiyan <[email protected]>

nbhuiyan force-pushed the invokedynamic-exception branch from b314536 to c67008b Compare July 12, 2023 01:54

0xdaryl approved these changes Jul 12, 2023

View reviewed changes

0xdaryl merged commit 5387910 into eclipse-openj9:master Jul 12, 2023

mpirvu mentioned this pull request Aug 10, 2023

JITServer throughput regression #17930

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Abort compilations for exceptions during invokedynamic resolution #17625

Abort compilations for exceptions during invokedynamic resolution #17625

nbhuiyan commented Jun 20, 2023 •

edited

Loading

nbhuiyan commented Jun 20, 2023

nbhuiyan commented Jun 20, 2023

0xdaryl Jun 20, 2023

0xdaryl Jun 20, 2023

0xdaryl Jun 20, 2023

0xdaryl commented Jun 21, 2023

nbhuiyan commented Jun 21, 2023

nbhuiyan commented Jun 21, 2023

0xdaryl commented Jun 21, 2023

nbhuiyan commented Jun 21, 2023

0xdaryl commented Jun 22, 2023

0xdaryl commented Jun 22, 2023

mpirvu left a comment

nbhuiyan commented Jun 22, 2023

mpirvu left a comment

mpirvu Jun 22, 2023

nbhuiyan Jun 23, 2023

nbhuiyan Jun 23, 2023

mpirvu Jun 23, 2023

mpirvu Jun 23, 2023

mpirvu Jun 23, 2023

nbhuiyan Jun 23, 2023

mpirvu commented Jun 22, 2023

nbhuiyan commented Jul 6, 2023

mpirvu left a comment

mpirvu commented Jul 10, 2023

mpirvu commented Jul 11, 2023

mpirvu commented Jul 11, 2023

nbhuiyan commented Jul 12, 2023 •

edited

Loading

0xdaryl commented Jul 12, 2023

	bool
	TR_ResolvedJ9Method::isUnresolvedCallSiteTableEntry(int32_t callSiteIndex)
	{
	return (j9object_t)callSiteTableEntryAddress(callSiteIndex) == NULL;
	}

Abort compilations for exceptions during invokedynamic resolution #17625

Abort compilations for exceptions during invokedynamic resolution #17625

Conversation

nbhuiyan commented Jun 20, 2023 • edited Loading

nbhuiyan commented Jun 20, 2023

nbhuiyan commented Jun 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xdaryl commented Jun 21, 2023

nbhuiyan commented Jun 21, 2023

nbhuiyan commented Jun 21, 2023

0xdaryl commented Jun 21, 2023

nbhuiyan commented Jun 21, 2023

0xdaryl commented Jun 22, 2023

0xdaryl commented Jun 22, 2023

mpirvu left a comment

Choose a reason for hiding this comment

nbhuiyan commented Jun 22, 2023

mpirvu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpirvu commented Jun 22, 2023

nbhuiyan commented Jul 6, 2023

mpirvu left a comment

Choose a reason for hiding this comment

mpirvu commented Jul 10, 2023

mpirvu commented Jul 11, 2023

mpirvu commented Jul 11, 2023

nbhuiyan commented Jul 12, 2023 • edited Loading

0xdaryl commented Jul 12, 2023

nbhuiyan commented Jun 20, 2023 •

edited

Loading

nbhuiyan commented Jul 12, 2023 •

edited

Loading