Modify SCC queries to take non-persistent parameters

[cjjdespres]

The high-level shared class cache interface currently supports a number of queries like

openj9/runtime/compiler/env/J9SharedCache.cpp

Line 716 in d15ef63

TR_J9SharedCache::offsetInSharedCacheFromROMClass(J9ROMClass *romClass)

These queries (offsetInSharedCacheFromROMMethod is another) are sometimes used directly, and sometimes form the basis of other queries like

openj9/runtime/compiler/codegen/J9AheadOfTimeCompile.cpp

Line 155 in d15ef63

J9::AheadOfTimeCompile::offsetInSharedCacheFromROMClass(TR_SharedCache *sharedCache, J9ROMClass *romClass)

A typical use of these methods is this:

openj9/runtime/compiler/codegen/J9AheadOfTimeCompile.cpp

Lines 552 to 554 in d15ef63

	TR_OpaqueClassBlock *inlinedMethodClass = resolvedMethod->containingClass();
	J9ROMClass *romClass = reinterpret_cast<J9ROMClass *>(fej9->getPersistentClassPointerFromClassPointer(inlinedMethodClass));
	uintptr_t romClassOffsetInSharedCache = self()->offsetInSharedCacheFromROMClass(sharedCache, romClass);

Whenever we use these methods we always have something like a TR_OpaqueClassBlock * (ultimately a J9Class * - that getPersistentClassPointerFromClassPointer immediately converts the opaque class block to a J9Class) or J9Method * on hand, that we then convert to persistent form (J9ROMClass or J9ROMMethod) to pass to the existing queries. Currently this is fine, but in #18301 I will not be able to rely on a local SCC being present. Instead, when compiling at the server I will have to look up "offsets" (JITServer AOT cache record IDs) using only cached client data and the JITServer AOT cache. The queries supported by these caches all require more information than something like a J9ROMClass - they effectively need a full J9Class * (or J9Method *).

To support #18301, this PR changes these query functions to take non-persistent JVM entities (TR_OpaqueClassBlock * coming from a J9Class *, for instance, rather than J9ROMClass). All we do now is convert them immediately to persistent form and look them up in the SCC, but in #18301 I will override this behaviour when we are compiling a method intended for the JITServer AOT cache, instead looking up the relevant information in the client session data or JITServer AOT cache using the extra information that these queries will now receive.

I'd argue that these new functions have a certain symmetry with the existing rememberClass(J9Class *clazz). That function takes a class and returns an offset to its persistent representation (J9ROMClass) inside the local SCC. The new functions will ask the matching question, "What is the offset of the persistent representation of the given class?", instead of current "What is the offset of this J9ROMClass *?".

---

An example of the kind of lookup that's available at the server is:

openj9/runtime/compiler/runtime/JITClientSession.hpp

Line 475 in d15ef63

const AOTCacheClassRecord *getClassRecord(J9Class *clazz, JITServer::ServerStream *stream, bool &missingLoaderInfo);

That lookup involves the data stored in the maps

openj9/runtime/compiler/runtime/JITClientSession.hpp

Line 504 in d15ef63

PersistentUnorderedMap<J9Class*, ClassInfo> _romClassMap;

and

openj9/runtime/compiler/runtime/JITServerAOTCache.hpp

Line 600 in d15ef63

PersistentUnorderedMap<ClassKey, AOTCacheClassRecord *, ClassKey::Hash> _classMap;

That ClassKey needs more than just a J9ROMClass as well:

openj9/runtime/compiler/runtime/JITServerAOTCache.hpp

Line 517 in d15ef63

struct ClassKey

Technically speaking, I could add a new cache that lets me look up the relevant data using only, say, a J9ROMClass *, but that seems less desirable, for reasons I mentioned in #18301 (comment).

---

Since this PR is motivated by #18301, it may change based on the ultimate requirements of that PR as I finish it. I will also need to modify #18301 to base it on this one, so this will need to be merged before #18301.

[cjjdespres]

Attn @mpirvu. The current draft only modifies a couple of queries related to ROM classes, but eventually I will need to modify the queries related to methods as well.

[cjjdespres]

The isROMClassInSharedCache query still exists, I should say, but this PR turns it into a private helper for the base TR_J9SharedCache class.

[mpirvu]

LGTM

[mpirvu]

@dsouzai Since this involves a SharedCache API change, I would appreciate a review from you as well. Thanks

[cjjdespres]

There is another change that might make things easier for me: currently rememberClass(clazz) returns a uintptr_t *, a pointer to the class chain that identifies clazz in the local SCC. That's if it succeeds - if it fails it returns NULL. Looking at the code, it seems like there are only two places where this being a class chain is actually used:

openj9/runtime/compiler/control/JITClientCompilationThread.cpp

Line 2140 in 7bbeb0f

uintptr_t numClasses = classChain[0] / sizeof(classChain[0]) - 1;

and

openj9/runtime/compiler/control/JITClientCompilationThread.cpp

Line 3160 in 7bbeb0f

uintptr_t numClasses = classChain[0] / sizeof(classChain[0]) - 1;

In both cases the actual structure of the chain is used simply to get the number of entries in the chain, but that can be calculated using the clazz itself - indeed, in #18301 I've already modified these two uses of the class chain so they instead call new frontend functions that perform this calculation.

Everywhere else, it seems as though we do one of the following with the class chain:

1. Check that it's not NULL and otherwise ignore the return value.
2. Immediately look up its offset in the SCC with offsetInSharedCacheFromPointer and use that offset for something.
3. Store the pointer in some data structure. When we next access that field, we do (1) or (2).

Ensuring that rememberClass returns a valid class chain at the server when we don't have a client SCC is possible, because we ultimately record a class chain at the JITServer AOT cache using a ClassChainSerializationRecord, which contains:

openj9/runtime/compiler/runtime/JITServerAOTSerializationRecords.hpp

Lines 198 to 212 in 7bbeb0f

	struct IdList
	{
	public:
	IdList(size_t length) : _length(length) { }
	size_t length() const { return _length; }
	const uintptr_t *ids() const { return _ids; }
	uintptr_t *ids() { return _ids; }
	static size_t size(size_t length) { return sizeof(IdList) + length * sizeof(uintptr_t); }
	private:
	const size_t _length;
	uintptr_t _ids[];
	};

a pointer to which is nearly a uintptr_t * value of the right format. The main issue is the following common pattern:

openj9/runtime/compiler/codegen/J9AheadOfTimeCompile.cpp

Lines 142 to 152 in 7bbeb0f

	uintptr_t
	J9::AheadOfTimeCompile::offsetInSharedCacheFromPointer(TR_SharedCache *sharedCache, void *ptr)
	{
	uintptr_t offset = 0;
	if (sharedCache->isPointerInSharedCache(ptr, &offset))
	return offset;
	else
	self()->comp()->failCompilation<J9::ClassChainPersistenceFailure>("Failed to find pointer %p in SCC", ptr);
	return offset;
	}

The isPointerInSharedCache (and the related offsetInSharedCacheFromPointer in the SCC API) call would require me to keep an additional map around that inverts the record ID -> class chain record map that currently exists at the server.

Instead, I think it would be cleaner to, for instance, have rememberClass return a bool that indicates whether or not it succeeded in recording the clazz, and also have it take an additional uintptr_t &offset parameter that can be used to communicate the offset to the caller if the caller is interested. Other schemes are possible of course. Then, in scenario (1) we'd be checking the bool, in scenario (2) we'd just use the offset directly, and in scenario (3) we'd again just store the offset directly and maybe on use we would verify that the offset is still valid.

Is there any reason why the two-step approach is currently used that would forbid doing it in this new way?

[dsouzai]

LGTM.

While there is now a slight discrepancy wrt to the other methods that call isROMStructureInSharedCache, it is likely that the others will also eventually be generalized to operate based on a nonpersistent pointer given that the goal is to have AOT without a local SCC. Additionally, the whole point of these APIs was to hide the complexity and implementation details of the transformation between a pointer and an offset, and furthermore, to abstract what an offset even means (no one should be manipulating offsets directly; they should always use this APIs).

Therefore, as long as the API maintains its purpose, namely a service to exchange between pointers and offsets (regardless of what either actually mean) in a consistent manner, it is fair to change specifics. This PR conforms to this constraint.

[mpirvu]

jenkins test sanity all jdk17

[cjjdespres]

I do still need to modify the J9Method/J9ROMMethod SCC functions, but that and the rememberClass changes could all be separate PRs if that's easier.

[dsouzai]

Is there any reason why the two-step approach is currently used that would forbid doing it in this new way?

I can't think of why rememberClass couldn't be updated in the manner you described; the method would still maintain all its functionality, but I guess instead the explicit pointer we now return an offset (which can really just be thought of as a token or key) that can be exchanged for the real pointer as needed.

I do still need to modify the J9Method/J9ROMMethod SCC functions, but that and the rememberClass changes could all be separate PRs if that's easier.

Yeah better for that to be a separate PR.

[mpirvu]

This failed on zLinux as before:

FAILED: testServerAOTCache
java.lang.AssertionError: There are no deserialized methods at the client.

@cjjdespres Does you openj9 source include the changes from #18792 ?

[cjjdespres]

You're right, this is based on a commit before #18792 was merged.

[mpirvu]

jenkins test sanity all jdk17

[cjjdespres]

It looks like the test failed on Z in the same way:

FAILED: testServerAOTCache
java.lang.AssertionError: There are no deserialized methods at the client.

[mpirvu]

"java.lang.AssertionError: There are no deserialized methods at the client." is seen for aarch64 pliux and zlinux

[cjjdespres]

A 100x grinder here of the nightly build with the test modified by #18835 (I think that's what setting the openj9 repo and branch does) on ppc64le_linux did not result in any failures of this kind. The only failure was a timeout in shutting down the server in a different sub-component of testJITServer_2.

[mpirvu]

Since the failures seen are not related to this PR, I am going to merge it.