-
Notifications
You must be signed in to change notification settings - Fork 738
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
Allow JITServer AOT cache stores and loads without a local SCC #18301
Conversation
Attn @mpirvu. |
0d314ec
to
8ca0846
Compare
Is this only temporary? Loading from local AOT is faster than loading from the server. |
I meant that they would ignore it while attempting the JITServer AOT load and relocation. The decision to load from the local SCC happens before all of this, I think, and I shouldn't need to change that. |
runtime/bcutil/ROMClassBuilder.cpp
Outdated
* ROMClasses. | ||
*/ | ||
if (context->isJITServerAOTCacheRequested()) { | ||
fixReturnBytecodes(_portLibrary, (J9ROMClass *)romClassBuffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pshipton It was suggested that you might know more about fixReturnBytecodes
. Here I want to use it even when a local SCC is not available. Is this safe to do, or is fixReturnBytecodes
not applied to non-shared classes for reasons other than efficiency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit fuzzy on the details. I think it happens anyway but when storing a class into the shared cache it needs to happen up front because the classes become read-only. @gacholio or @TobiAjila ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling fixReturnBytecodes() does have the cost of evaluating all the bytecodes to figure out what the return type should be. I think outside the shared cache it just modifies it on the fly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Classes in the SCC are read-only, so they must have fixReturnBytecodes done before storing. It's safe to call it when classes are not going into the SCC, but it's unnecessary - the return bytecodes are fixed up on the fly (we assume the bytecodes are "correct" due to verification).
Just to review the changes needed in the deserializer (which I'll put in the design document too): Currently, when serving a cached JITServer AOT method, the server will send the method itself (including all of the normal relocation records generated for it) and all the JITServer AOT cache records associated with the method. These JITServer AOT cache records are effectively used to "relocate the relocation records" (as Irwin put it). After the client receives this data, the deserializer at the client will:
After this, the relocation runtime can relocate the method as usual, as the offsets in the record will now be valid with respect to the local SCC. (A side note at this point: Alexey pointed out that we could just use the JITServer AOT cache record ids as offsets when compiling a method at the server for storage in the JITServer AOT cache. This would let us skip step (3) above in conjunction with the deserializer changes I talk about below, once I get to modifying the code to allow for fresh JITServer AOT cache compilations without a local SCC.) What we instead need to do in the deserializer is:
Once that is done, the relocation work can proceed, which will involve creating a class derived from |
|
||
if (offset) | ||
auto *vmInfo = TR::compInfoPT->getClientData()->getOrCacheVMInfo(stream); | ||
if (offset || (aotCacheLoad && !vmInfo->_hasSharedClassCache)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This aotCacheLoad
parameter that I use here should only be temporary - when we ignore the local SCC during fresh JITServer AOT cache compilations this check will need to be updated, because there will be no local offset to store.
One problem that I've observed with my solution to the ROM classes differing is that if a local SCC is created without I haven't checked exactly how they differ, but I assume it's the debug information again. This isn't immediately a problem, I suppose - the client just has to run with a local SCC that was created with some option that forces a consistent ROM class representation (as If the debug info being inlined or not is irrelevant for correctness, then we could solve this by convert ROM classes to a known, consistent representation during comparison so we don't have to deal with this issue. There's already If the debug info being inlined or not is relevant (and so we don't want ROM classes that differ only in this respect to be equivalent to each other) then we either have to tolerate this problem or find a way of allowing debug info to be stored out-of-line when no local SCC exists. We would do this so that the ROM classes would still have a uniform representation, just one with debug info that's always out of line when using the AOT cache (instead of always inline as I have it in the draft PR). I did see in the code a comment mentioning that this can be done "when the allocation strategy [of the I should note that |
8ca0846
to
a715e17
Compare
Force pushed to rebase onto master to incorporate the changes in #18344. I also changed a few things so that the PR works when a
|
a715e17
to
8bc3f94
Compare
Force pushed to remove one instance of |
TR::Monitor *const _methodMonitor; | ||
|
||
PersistentUnorderedMap<uintptr_t/*ID*/, uintptr_t/*SCC offset*/> _classChainMap; | ||
PersistentUnorderedMap<uintptr_t/*ID*/, uintptr_t * /*deserializer chain*/> _classChainMap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These chains (and the ones in the well-known classes map) could just be vectors, I suppose.
I've pushed the deserializer changes, and the start of the JITServer AOT cache documentation in @AlexeyKhrabrov The actual AOT deserializer changes were more or less as discussed in the related issue, including (perhaps temporarily) removing support for reloading of classes and such. |
throw std::bad_alloc(); | ||
|
||
deserializerChain[0] = chainSize; | ||
memcpy(deserializerChain + 1, record->list().ids(), record->list().length()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect, incidentally. The idAndType
should be written to the chain, not the IDs themselves.
2edd393
to
5a1b3cf
Compare
The latest changes implement the alternate SCC frontend that ignores the local SCC (if present) and instead looks up the information cached by the deserializer in response to particular queries. The last thing that potentially needs to be done before this is ready is to deal with the |
There is one other thing - a deserializer reset (which happens when a client disconnects from a server) can happen concurrently. We have to make sure that such a reset cannot occur during relocation, or we have to detect during relocation that such a reset happened and abort relocation if it did. |
I'm not especially happy with
Once I finish with the fresh compilation portion of this issue, the deserializer won't have to update the offsets in a method at all, as the server should simply be able to use the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a proper review, just some notes and a few things I noticed in the code.
The actual AOT deserializer changes were more or less as discussed in the related issue, including (perhaps temporarily) removing support for reloading of classes and such.
I haven't had a chance to look at the code in detail (and won't have time for it for a while). At a high level, how does the new implementation behave when a class is unloaded and later loaded again? By the way, I've seen applications (e.g. some workloads in the Renaissance benchmark suite) where such class re-loading does seem to happen (although I'm not sure how much of the overall performance the affected methods are responsible for).
There is one other thing - a deserializer reset (which happens when a client disconnects from a server) can happen concurrently. We have to make sure that such a reset cannot occur during relocation, or we have to detect during relocation that such a reset happened and abort relocation if it did.
This existing mechanism and its API turned out to be rather awkward when building other things on top of the deserializer (in my current work on early JIT compilation). This PR might be a good opportunity to get rid of it rather than support it in new code. One likely better mechanism I can think of is a reader-writer lock where threads performing AOT deserialization+load are readers and threads doing a reset are writers. Although I haven't figured the details. If we do keep the current reset detection mechanism, throwing an exception instead of propagating the wasReset
flag would probably be a better API for it.
|
||
|
||
#if (defined(J9VM_ARCH_X86) || defined(J9VM_ARCH_S390) || defined(J9VM_ARCH_POWER) || defined(J9VM_ARCH_AARCH64)) && defined(J9VM_OPT_JITSERVER) | ||
// Regardless of whether or not the local SCC is enabled, we still need to enable portable AOT if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? One of the advantages of JITServer AOT cache is transparent handling of different client environments without sacrificing throughput for portability. I believe this should still be at least configurable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change was @mpirvu's suggestion. It was in the context of a discussion we were having about the TR_AOTHeader
differing depending on whether or not -Xshareclasses
was enabled or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also still in. I've made a note to revisit this decision. We could at least in future add a test of whether or not the new implementation is being used, and to enable the portable shared cache flag only if it is. I can't quite remember exactly what relo header differences I was getting, but I do remember that J9_EXTENDED_RUNTIME2_ENABLE_PORTABLE_SHARED_CACHE
wasn't necessary to eliminate them all - it was only sufficient. So it might not be necessary to enable even in the new implementation if forcing portable AOT is considered that bad. But I'd have to look at what exactly was going wrong again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be fixed eventually, it's a performance degradation. I remember seeing measurable reductions in throughput with portable AOT back when I worked on the original AOT cache implementation. There is also no point in enabling portable AOT by default at all when running in a container without local SCC (but with AOT cache). Could you please open an issue for this?
I still don't understand what could be different in the TR_AOTHeader
depending on whether local SCC is enabled. Maybe there is something subtle going wrong with either featureFlags
or processDescription
. Have you tried testing this both inside and outside a container, and with portable AOT explicitly enabled or disabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially I had individual overrides in particular places, because having/not having an SCC would change some VM properties at startup. Those solved all the header incompatibilities. Explicitly setting the relocatable processor/CPU description in rossa.cpp
was one of them. Changing how the compressed pointer shift amount was determined was another, as that changes if we're in containers and using an SCC (from what I remember).
Setting the portable AOT flag happened to fix all those individual issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll open up an issue
@@ -366,6 +367,14 @@ class ROMClassCreationContext | |||
} | |||
|
|||
bool canPossiblyStoreDebugInfoOutOfLine() const { | |||
if (isJITServerAOTCacheRequested()) { | |||
/* | |||
* If the JITServer AOT cache might be used then debug information should be forced inline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this change interact with zeroing out SRPs to debug info entries in JITServerHelpers::packROMClass()
? If debug info is always stored inline, may as well keep SRPs to it in serialized ROMClasses. Might also help with the problem described here #18301 (comment).
@@ -34,30 +34,54 @@ enum TableKind { Loader, Chain, Name }; | |||
// struct is linked into three linked lists - one for each hash table in TR_PersistentClassLoaderTable. | |||
struct TR_ClassLoaderInfo | |||
{ | |||
TR_PERSISTENT_ALLOC(TR_Memory::PersistentCHTable) | |||
// Deleting because these records are variably-sized | |||
TR_ClassLoaderInfo(const AOTSerializationRecord &) = delete; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong parameter type.
|
||
void *const _loader; | ||
TR_ClassLoaderInfo *_loaderTableNext; | ||
void *const _chain; | ||
TR_ClassLoaderInfo *_chainTableNext; | ||
#if defined(J9VM_OPT_JITSERVER) | ||
TR_ClassLoaderInfo *_nameTableNext; | ||
size_t _nameLength; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could store it as J9UTF8 *
instead to avoid the copy when there is a local SCC and the 1st loaded ROMClass with its name string is stored there. Might also simplify some code.
// Delay relocation by default, unless this option is enabled | ||
if (!comp->getOption(TR_DisableDelayRelocationForAOTCompilations)) | ||
return false; | ||
|
||
#if defined(J9VM_OPT_JITSERVER) | ||
if (comp->isDeserializedAOTMethod() && comp->getPersistentInfo()->getJITServerAOTCacheDelayMethodRelocation()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the command line option be removed then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to update this - I haven't removed the option in this final version of the PR, because we still have the option of running with the old implementation.
int32_t numSuperclasses = TR::Compiler->cls.classDepthOf(fe()->convertClassPtrToClassOffset(clazz)); | ||
int32_t numInterfaces = numInterfacesImplemented(clazz); | ||
int32_t numSuperclasses = fe()->numSuperclasses(clazz); | ||
int32_t numInterfaces = fe()->numInterfacesImplemented(clazz); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this log message is worth iterating through interfaces twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since numSuperclasses
and numInterfaces
are used a little later on to fill in the class chain, I got rid of the necessaryClassChainLength
call instead.
_chain(chain), _chainTableNext(NULL), | ||
_nameLength(J9UTF8_LENGTH(nameStr)), _nameTableNext(NULL) | ||
{ | ||
memcpy(&_nameData, J9UTF8_DATA(nameStr), J9UTF8_LENGTH(nameStr)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary (along with memory allocation) if not using AOT cache.
@@ -127,13 +123,23 @@ class JITServerAOTDeserializer | |||
// looked up using the cached SCC offsets if the class was unloaded. | |||
J9Class *getRAMClass(uintptr_t id, TR::Compilation *comp, bool &wasReset); | |||
|
|||
std::vector<J9Class *> getRAMClassChain(TR::Compilation *comp, J9Class *clazz); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be a scratch vector or a statically sized buffer (class chain length is limited).
5a1b3cf
to
d768cc2
Compare
Force-pushed to bring in the changes from #18585 and #18568. I also added a commit that strips the debug info from ROM classes and fixes the bytecodes during ROM class packing, so I was able to drop the changes to the VM and ROM class building that I had before. Stripping the debug info out was one solution to the problem I mentioned in #18301 (comment). The implementation ended up being a little trickier than I anticipated. @AlexeyKhrabrov I'll be getting to your initial remarks next. About the class unloading and loading - the new implementation still marks the class as unloaded in the Moving to a reader-writer lock system sounds fine as well. It wouldn't be hard to write one myself, but is there an implementation already in openj9? |
One thing I've run into in the other half of this issue (code generation at the server) is the use of SCC methods like
Both sources require more information than simply a A somewhat less invasive change would be to cache some kind of This sort of problem will probably arise in the other SCC lookup methods - the server caches generally want more information than what is typically given to these methods. The |
d768cc2
to
a98741d
Compare
jenkins test sanity plinuxjit,xlinuxjit,zlinuxjit,alinux64jit jdk17 |
plinuxjit shows the following crash in compiled code:
and
On zLinuxjit we have:
and
On xlinuxjit we have
and
|
Just to record it here - the test build failures are likely related to the PR that #18981 reverted. Once this PR is rebased to bring in that revert and the ROM class walk fix, we should be good to try PR testing again. |
Signed-off-by: Christian Despres <[email protected]>
The new reset detection mechanism renders it unnecessary. Signed-off-by: Christian Despres <[email protected]>
The -XX:[+|-]JITServerAOTCacheIgnoreLocalSCC option explicitly enables or disables the use of the local SCC when compiling a method using the JITServer AOT cache. If disabled, a local SCC must be present and have sufficient writable space for the JITServer AOT cache to be used. By default this option is disabled. Signed-off-by: Christian Despres <[email protected]>
The persistent class loader table will now track the names of the first-loaded classes of class loaders without associated chains being present. Signed-off-by: Christian Despres <[email protected]>
Signed-off-by: Christian Despres <[email protected]>
The J9_EXTENDED_RUNTIME2_ENABLE_PORTABLE_SHARED_CACHE flag causes the VM to make a number of conservative assumptions during code generation, including the hardware capabilities available. When the JITServer AOT cache is used, these assumptions should be made regardless of whether or not a local SCC exists. This allows JITServer AOT cache sharing to occur between clients that may or may not have a local SCC; their headers would otherwise differ and so prevent cache hits. Signed-off-by: Christian Despres <[email protected]>
A JITServer client will now request an AOT load even if a local SCC is not present. When the relocation mechanism and deserializer have been modified appropriately, this will allow JITServer AOT cache loads to occur in that circumstance. Both the server and client will now recognize AOT cache load requests even if the method is not eligible for an AOT cache store. Signed-off-by: Christian Despres <[email protected]>
Signed-off-by: Christian Despres <[email protected]>
When ignoring the local SCC, we now relocate deserialized methods using the new TR_J9DeserializerSharedCache, which consults the underlying JITServerNoSCCAOTDeserializer for offset queries. Signed-off-by: Christian Despres <[email protected]>
Signed-off-by: Christian Despres <[email protected]>
Signed-off-by: Christian Despres <[email protected]>
The _ignoringLocalSCC property is true at a JITServer client when the compilation is to be stored in the JITServer's AOT cache and the client is using the new deserializer implementation (hence ignoring its local SCC). Since the server is responsible for generating relocation and AOT cache records in these circumstances, this property is used to stop certain SCC-related checks from being performed and stop relo records from being generated when it is true. Signed-off-by: Christian Despres <[email protected]>
A client running with _JITServerAOTCacheIgnoreLocalSCC will now request AOT cache stores if the compilation is eligible for them. The server will send the result to the client with a new AOTCache_storedAOTMethod, and the client will deserialize it using the new deserializer implementation. Signed-off-by: Christian Despres <[email protected]>
This saves a little network traffic, and also avoids an assert firing in ClientSessionData::getClassRecord(). Signed-off-by: Christian Despres <[email protected]>
Signed-off-by: Christian Despres <[email protected]>
Signed-off-by: Christian Despres <[email protected]>
2fd35ec
to
4f458dd
Compare
Force-pushed to rebase onto master. |
jenkins test sanity plinuxjit,xlinuxjit,zlinuxjit,alinux64jit jdk17 |
zlinux failed the |
// If we are using the JITServer AOT cache and ignoring the local SCC, we need to remember the name of clazz | ||
// with or without chain. Otherwise (not using AOT cache or not ignoring the local SCC) there is no point in continuing | ||
// without a chain. | ||
if (!chain && (!useAOTCache || !_persistentMemory->getPersistentInfo()->getJITServerAOTCacheIgnoreLocalSCC())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't compile without defined(J9VM_OPT_JITSERVER)
, e.g. on macOS; see failed build:
[2024-02-20T21:51:46.296Z] /Users/jenkins/workspace/Build_JDK22_aarch64_mac_Personal/openj9/runtime/compiler/env/ClassLoaderTable.cpp:239:76: error: no member named 'getJITServerAOTCacheIgnoreLocalSCC' in 'TR::PersistentInfo'
[2024-02-20T21:51:46.296Z] if (!chain && (!useAOTCache || !_persistentMemory->getPersistentInfo()->getJITServerAOTCacheIgnoreLocalSCC()))
[2024-02-20T21:51:46.296Z] ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
[2024-02-20T21:51:46.296Z] 1 error generated.
[2024-02-20T21:51:46.296Z] make[6]: *** [runtime/compiler/CMakeFiles/j9jit.dir/env/ClassLoaderTable.cpp.o] Error 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll put a fix shortly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some concerns about changes in control logic, a few questions, and some suggestions for possible future refactoring.
// This record ID can only be missing from the cache if it was removed by a concurrent reset | ||
// TODO: is this true any more? The deserializerWasReset above should guarantee that we haven't | ||
// reset by this point. | ||
wasReset = true; | ||
return V(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be an assertion then.
@@ -992,6 +997,11 @@ TR_ResolvedRelocatableJ9Method::TR_ResolvedRelocatableJ9Method(TR_OpaqueMethodBl | |||
{ | |||
TR_J9VMBase *fej9 = (TR_J9VMBase *)fe; | |||
TR::Compilation *comp = TR::comp(); | |||
#if defined(J9VM_OPT_JITSERVER) | |||
if (fej9->_compInfoPT->getMethodBeingCompiled()->_useAOTCacheCompilation) | |||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can we skip this when ignoring local SCC?
@@ -1856,7 +1877,7 @@ TR_ResolvedRelocatableJ9Method::createResolvedMethodFromJ9Method(TR::Compilation | |||
isSystemClassLoader) | |||
{ | |||
resolvedMethod = new (comp->trHeapMemory()) TR_ResolvedRelocatableJ9Method((TR_OpaqueMethodBlock *) j9method, _fe, comp->trMemory(), this, vTableSlot); | |||
if (comp->getOption(TR_UseSymbolValidationManager)) | |||
if (!ignoringLocalSCC && comp->getOption(TR_UseSymbolValidationManager)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can we skip this when ignoring local SCC?
@@ -1039,11 +1039,13 @@ JITServerAOTCache::storeMethod(const AOTCacheClassChainRecord *definingClassChai | |||
"AOT cache %s: method %s @ %s index %u class ID %zu AOT header ID %zu already exists", | |||
_name.c_str(), signature, levelName, index, definingClassId, aotHeaderRecord->data().id() | |||
); | |||
return false; | |||
methodRecord = it->second; | |||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the logic compared to the previous implementation: we return an existing serialized method instead of the freshly compiled one (which gets discarded). Is this always the right thing to do? For instance, is it possible that the client requests a fresh AOT compilation because it failed to load an existing server-cached version, and now will receive the same bad old version after a (discarded) fresh compilation? After looking at how the various store/load flags are handled, I think it is possible, but I'm not entirely sure.
Also, returning true
here contradicts the doc comment in the .hpp file.
@@ -214,6 +214,25 @@ class ServerStream : public CommunicationStream | |||
TR_VerboseLog::writeLineLocked(TR_Vlog_JITServer, "Could not finish compilation: %s", e.what()); | |||
} | |||
} | |||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: could simply add a msgType
parameter to finishCompilation()
instead of adding a new method, which would also reduce code duplication at the call sites in outOfProcessCompilationEnd()
.
@@ -1010,7 +1010,7 @@ JITServerAOTCache::storeMethod(const AOTCacheClassChainRecord *definingClassChai | |||
TR_Hotness optLevel, const AOTCacheAOTHeaderRecord *aotHeaderRecord, | |||
const Vector<std::pair<const AOTCacheRecord *, uintptr_t/*reloDataOffset*/>> &records, | |||
const void *code, size_t codeSize, const void *data, size_t dataSize, | |||
const char *signature, uint64_t clientUID) | |||
const char *signature, uint64_t clientUID, const CachedAOTMethod *&methodRecord) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: methodRecord
is a confusing name, there are actual method records.
{ | ||
aotCacheStore = entry->_useAOTCacheCompilation && | ||
compInfo->methodCanBeJITServerAOTCacheStored(compiler->signature(), compilee->convertToMethod()->methodType()); | ||
aotCacheLoad = persistentInfo->getJITServerUseAOTCache() && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this changes the semantics of the -Xaot:jitserverAOTCacheStoreExclude=
option, which previously implied LoadExclude
as well (and still implies for the local SCC mode).
if (aotCacheLoad) | ||
deserializer->incNumCacheMisses(); | ||
|
||
auto method = SerializedAOTMethod::get(methodStr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the code duplicated in the next branch can be factored out.
@@ -3585,7 +3678,7 @@ remoteCompile(J9VMThread *vmThread, TR::Compilation *compiler, TR_ResolvedMethod | |||
} | |||
} | |||
|
|||
if (!compiler->getOption(TR_DisableCHOpts) && !useAotCompilation && !compiler->isDeserializedAOTMethod()) | |||
if (!compiler->getOption(TR_DisableCHOpts) && !useAotCompilation && (compiler->isDeserializedAOTMethodStore() || !compiler->isDeserializedAOTMethod())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having trouble understanding this condition. Shouldn't isDeserializedAOTMethodStore()
imply that this is an AOT compilation?
// TODO: is -Xaot:forceaot recognized when -Xshareclasses:none is specified? | ||
canDoRelocatableCompile = true; | ||
} | ||
else if (!preferLocalComp(entry)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to factor this logic out (although to be fair, this whole method is a bit of a mess and could use some much heavier refactoring...).
These changes finish resolving #16721 and allow clients of a JITServer to request JITServer AOT cache load or stores regardless of whether or not a client has a local SCC available. The new implementation (which bypasses the local SCC entirely) is controlled by a new option
-XX:[+|-]JITServerAOTCacheIgnoreLocalSCC
, which is off by default.For a high level overview of the changes in their entirety, see #16721 (comment). In a future PR I will expand that comment into a description of the JITServer AOT cache for the
doc/
folder.