Add JITServer AOT cache persistence #15949
Conversation
|
The |
cjjdespres
force-pushed
the
jitserver-aot-persist
branch
from
28abaf0
to
14741df
Compare
cjjdespres
force-pushed
the
jitserver-aot-persist
branch
2 times, most recently
from
0183e8b
to
aed120c
Compare
|
My intent was to split up the header in two parts, one with the initial versioning information that can be read to ensure cache compatibility, and one that has more concrete cache data (map sizes, offsets, checksum). Maybe from that perspective the types should look like struct JITServerAOTCacheVersion
{
uint32_t snapshotVersion;
uint32_t jitserverVersionMajor;
uint32_t jitserverVersionMinor;
uint32_t jitserverVersionPatch;
uint64_t serverUID;
};
struct JITServerAOTCacheHeader
{
size_t numClassLoaderRecords;
size_t numClassRecords;
size_t numMethodRecords;
size_t numClassChainRecords;
size_t numWellKnownClassesRecords;
size_t numAOTHeaderRecords;
size_t numCachedAOTMethods;
uint32_t checksum;
<various offsets, potentially>
};My instinct was to have the snapshot preamble be human-readable, something like but maybe the encoding should be more compact? |
It's up to you. You may have to do some conversions ASCII <--> integer if you go this route. |
I second Marius' suggestion of an 8-byte eye-catcher field with a character sequence "AOTCACHE", I think it's much simpler than having a human readable header where we need to parse decimal integers (unless there is some specific advantage in doing that). One more thought about the header and versioning. In addition to the JITServer protocol version, the code also uses configuration/compatibility flags (see |
|
For the |
Yes, the |
cjjdespres
force-pushed
the
jitserver-aot-persist
branch
from
aed120c
to
8ffd9d5
Compare
| return true; | ||
| } | ||
|
|
||
| static bool |
There was a problem hiding this comment.
We should consider merging SerializedAOTMethod and CachedAOTMethod into the record class hierarchies to avoid duplicating this code (there will probably be more places). Not in this PR though.
cjjdespres
force-pushed
the
jitserver-aot-persist
branch
from
8ffd9d5
to
1ef800b
Compare
|
I was thinking that for reading-in serialization records we can skip writing new constructors and just copy data from the file into the memory allocated for the record. Something like: |
Note that you still have to construct Since we know the record type (e.g. |
|
I forgot that |
cjjdespres
force-pushed
the
jitserver-aot-persist
branch
2 times, most recently
from
3a3d7f6
to
5425355
Compare
|
jenkins test sanity plinuxjit,xlinuxjit jdk17 |
It looks like there is a null sub-record somewhere, which should be impossible during normal operation. Even after this issue is fixed, we'll still need to check for invalid sub-record IDs that point to nulls in ID-record vectors so that JITServer doesn't crash after loading a malformed snapshot file. |
| (header.classLoaderId() >= classLoaderRecords.size())) | ||
| return NULL; | ||
|
|
||
| auto record = new (AOTCacheRecord::allocate(size(header.nameLength()))) AOTCacheClassRecord(classLoaderRecords[header.classLoaderId()]); |
There was a problem hiding this comment.
For instance, there is a missing check that classLoaderRecords[header.classLoaderId()] is non-null here. The same applies to all other places where we set sub-record pointers.
cjjdespres
force-pushed
the
jitserver-aot-persist
branch
from
f6b70c3
to
6f0ed78
Compare
|
I'll see if the cache fails to be read with these changes. |
cjjdespres
force-pushed
the
jitserver-aot-persist
branch
from
6f0ed78
to
48839ea
Compare
Sure, but this still only hides the original issue that lead to a segfault. If the cache is populated and persisted correctly, there should be no missing sub-records when it gets loaded. |
|
Certainly, we ought to be able to read a cache we just wrote. It appears that reading fails at this block case AOTSerializationRecordType::WellKnownClasses:
if ((sccOffset.recordId() >= wellKnownClassesRecords.size()) || !wellKnownClassesRecords[sccOffset.recordId()])
goto error;
record->records()[i] = wellKnownClassesRecords[sccOffset.recordId()];
break;The Evidently, at some point a cached aot method was created by the server that references a second well known classes record, and this cached aot method was saved during serialization but the corresponding well known classes record was not. |
|
This is strange anyway, since we save the sizes like this: {
OMR::CriticalSection cs(_wellKnownClassesMonitor);
header._numWellKnownClassesRecords = _wellKnownClassesMap.size();
header._nextWellKnownClassesId = _nextWellKnownClassesId;
}and I think that |
|
Unfortunately, I didn't save the logs for the server when I created that particular cache file. I just created a second one with each type of record and it seems to load without issue, though a short time later the acmeair client consistently experiences a segfault: |
This doesn't look good. On top of the invalid sub-records, it looks like some of the data in the serialization records loaded from the file that gets sent to the client is inconsistent. There is some debug logging in |
| @@ -338,6 +477,9 @@ class JITServerAOTCache | |||
| const uintptr_t _includedClasses; | |||
| }; | |||
|
|
|||
| static WellKnownClassesKey getRecordKey(const AOTCacheWellKnownClassesRecord *record) | |||
| { return { record->records(), record->data().list().length() }; } | |||
There was a problem hiding this comment.
I think this may be the problem. Notice that the key initializer that this replaces above is { record->records(), length, includedClasses }. This is likely the cause of the discrepancies I saw in #15949 (comment), and might be the cause of the segfault as well.
There was a problem hiding this comment.
Yeah that's probably it. This could have been prevented by defining actual constructors in the ...Key structs, which is still worth doing.
Signed-off-by: Christian Despres <despresc@ibm.com>
cjjdespres
force-pushed
the
jitserver-aot-persist
branch
from
48839ea
to
43604b4
Compare
|
That particular change does fix the discrepancy (and subsequent cache read failure) of #15949 (comment). With that change, a cache with each type of record will round trip to an equal cache and subsequently operate correctly, as long as the segfault in #15949 (comment) doesn't occur just after client start-up. That change does not fix the segfault, which may be unrelated to this PR. Even with a JDK built using
The acmeair client will frequently, but not always, segfault after (4). |
I'll try to reproduce this with a recent build. I remember I have run AcmeAir with AOTServer AOT cache (but without |
|
0.33.1 crashes too if |
|
Does it crash inside a compiled method? If so, is it the same method every time? Does it look similar to #15146 by any chance? |
|
In one such run the stacktrace from the javacore indicates a problem during stack walking by GC: |
|
Since the segfault appears to be unrelated to AOT cache persistence and is only reproducible with a non-standard option |
|
jenkins test sanity plinuxjit,xlinuxjit,zlinuxjit jdk17 |
|
Timeout on x86 for cmdLineTester_jvmtitests_debug_5 On zLinux we have: |
|
jenkins test sanity xlinuxjit,zlinuxjit jdk17 |
The current JITServer AOT cache lacks any persistence mechanism. These changes add two functions
to the public cache interface that will read or write an entire cache to or from a particular file stream. A cache file header and versioning mechanism has also been introduced.
The added intrusive linked list traversals of the AOT cache maps allow cache serialization to be performed concurrently with other cache record storage and retrieval.