Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use VM criteria for J2I Thunk Sharing in AOT Compiles #16625

Merged
merged 3 commits into from
Feb 2, 2023

Conversation

dsouzai
Copy link
Contributor

@dsouzai dsouzai commented Jan 30, 2023

In a non-AOT compilation, the JIT generates J2I Thunks and stores them into a VM Hash Table using the j9ThunkNewSignature API; it looks up existing thunks using the j9ThunkLookupSignature API. In an AOT compilation, the JIT does not uses these APIs, but rather simply stores the Thunks into the SCC using the signature of the method as the key.

The VM API will share the thunks for two different methods if all of the following is true:

  1. They have the same return type
  2. They have the same primitive type arguments

Therefore, if two methods have different object types, they can still use the same thunk. For example, foo(IILMyObject;)V and bar(IILMyOtherObject;)V can share the same thunk, but baz(ILMyObject;I)V cannot.

Because AOT compiles only used the signature of the method as the key for the SCC entry, it resulted in thunks that could be shared not being shared. This is the root cause of the problem described in #16402.

This PR reuses the VM mechanism to encode the signature to generate the key to store the SCC entry. As such, the same sharing that occurs in a non-AOT compile will also occur in an AOT compile.

Fixes #16402

@@ -9195,13 +9195,44 @@ TR_J9SharedCacheVM::getDesignatedCodeCache(TR::Compilation *comp)
void *
TR_J9SharedCacheVM::getJ2IThunk(char *signatureChars, uint32_t signatureLength, TR::Compilation *comp)
{
return (void *)findPersistentThunk(signatureChars, signatureLength);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a future PR where findPersistentThunk will be cleaned up. At the moment, two different types of J2I thunks are stored using this, but they are maintained by completely different data structures. The future cleanup PR will address this.

@dsouzai
Copy link
Contributor Author

dsouzai commented Jan 30, 2023

@mpirvu could you please review.

@pshipton could you please review the changes in thunkcrt.c.

Copy link
Contributor

@mpirvu mpirvu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I only have a couple of small requests regarding comments.
If you do any future cleanup (I saw a comment saying that you intend that) maybe it's worth creating a data structure for the thunk. I saw some magic where we get back by 8 bytes and then read the thunk code length stored on 4 bytes.

runtime/codert_vm/thunkcrt.c Outdated Show resolved Hide resolved
runtime/codert_vm/thunkcrt.c Show resolved Hide resolved
@mpirvu mpirvu self-assigned this Jan 31, 2023
Refactor the part of j9ThunkEncodeSignature that determines the encoding
of the J2I Thunk Signature so that it can be reused by other functions.

Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>
@mpirvu
Copy link
Contributor

mpirvu commented Feb 1, 2023

jenkins test sanity all jdk17

Add methods that will find and store J2I Thunks into the SCC rather than
the hash tables. Because the SCC key needs to be a UTF8 string, generate
the encoding of the signature (used as the key for the SCC entry)
in ASCII.

Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>
Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>
@mpirvu
Copy link
Contributor

mpirvu commented Feb 1, 2023

@dsouzai Some compilation errors:

11:03:31  /Users/jenkins/workspace/Build_JDK17_x86-64_mac_Personal/openj9/runtime/codert_vm/thunkcrt.c:584:59: error: passing 'U_8 *' (aka 'unsigned char *') to parameter of type 'const char *' converts between pointers to integer types with different sign [-Werror,-Wpointer-sign]
11:03:31          store = vm->sharedClassConfig->storeSharedData(vmThread, encodedSignature, encodedLength, &dataDescriptor);
11:03:31                                                                   ^~~~~~~~~~~~~~~~
11:03:31  /Users/jenkins/workspace/Build_JDK17_x86-64_mac_Personal/openj9/runtime/codert_vm/thunkcrt.c:608:50: error: passing 'U_8 *' (aka 'unsigned char *') to parameter of type 'const char *' converts between pointers to integer types with different sign [-Werror,-Wpointer-sign]
11:03:31          vm->sharedClassConfig->findSharedData(vmThread, encodedSignature, encodedLength, J9SHR_DATA_TYPE_AOTTHUNK, FALSE, &dataDescriptor, NULL);
11:03:31                                                          ^~~~~~~~~~~~~~~~
11:03:31  2 errors generated.

@dsouzai
Copy link
Contributor Author

dsouzai commented Feb 1, 2023

I guess some compilers don't like encodedSignature not getting cast to a const char * :(

@dsouzai
Copy link
Contributor Author

dsouzai commented Feb 1, 2023

Since tests were already kicked off, I'll wait to force push until they're done.

zLinux build failed because of #9758

@dsouzai
Copy link
Contributor Author

dsouzai commented Feb 1, 2023

jenkins test sanity all jdk17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Too many AOT load failures with -Xaot:forceaot
3 participants