Optimize getMethodFromName calls in J9VMServer

[KavinSatheeskumar]

Optimize fe->getMethodFromName(...)

Replace getMethodFromName on J9VMServer with the
base implementation, and add caching for getMethodFromClass
to reduce the number of total messages.

[KavinSatheeskumar]

Note, it will be a while before this is merged, as testing needs to be done to demonstrate an actual performance improvement.

[KavinSatheeskumar]

A quick explanation

Context

One of the VM methods that JITServer requires getMethodFromName, when this method is called on the JITServer sends a VM_getMethodFromName message to the client, where it calls the method and returns the value.

It seems like all calls to this method just reference hard-coded strings. However, from this issue (https://github.ibm.com/runtimes/rt-tr-control-repo/issues/24), it seems like this message is sent hundreds of times. While this is a very small amount in the grand scheme of things

1. In my testing, it can occasionally occupy a larger fraction of the total messages sent (up to 1%)
2. This fix is incredibly easy.
3. It may be a proof of concept for other optimizations

Actual Implementation

1. Replace the J9VMServer.cpp implementation of getMethodFromName with the default, causing it to rely on 2 other front end queries
   • getSystemClassFromClassName
   • getMethodFromClass

The first of these calls is already cached, so we add a caching mechanism for the second

2. We introduce a hash map which maps { J9Class*, std::string } -> { J9Method* }

   • We store this map in the ClientSessionData object
   • This map is called _methodByNameMap

3. When getMethodFromClass is called

   • It checks the _methodByNameMap to see if this call has been cached
   • if it has, it returns the value
   • otherwise it sends a message to the client
   • if the returned method 1. has the system class loader as its class loader, 2. is not being called from another class
     • It stores the value in the cache
   • It returns the J9Method* returned by the client

4. When classes get unloaded

   • If the unloaded class is known to have the system class loader as its class loader
     • the _methodByName map is searched and all invalid cache entries are removed

[mpirvu]

Does the code run? You are missing the initialization of the newly created monitor which should cause a crash.
Also, your code always creates a local copy of the new hashtable, thus the hit rate is going to be 0.
I am also concerned about the overhead of scanning the entire hashtable for every class that gets unloaded (O(m*n)) while holding the romMapMonitor. This is probably the most contended monitor at the server.

[mpirvu]

Apart from the smaller inline suggestions, there are some bigger concerns with the current implementation.

1. A name does not uniquely determine a method. You can have a class being loaded by two different classloaders, so the resulting j9class entities will have the same class name (and same method names) but their are considered different otherwise. If you look at the existing code, when we retrieve a class from its name we also specify the classloader (e.g PersistentUnorderedMap<ClassLoaderStringPair, TR_OpaqueClassBlock*> _classBySignatureMap;
2. Not all classes are cached by the server. Your code could cache a {name --> j9method} mapping and the code that needs to delete the mapping (because of a class unload) will not be able to find it (because you cannot find the j9class, so you cannot find the name).
3. For a large application there could be a very large number of methods cached and the keys (composed of 3 std::strings) could also be large. This may end-up taking too much memory, especially relative to the improvement that this feature is going to provide (TBD).

[mpirvu]

Ignoring the functional issues mentioned above, it would be interesting to get a feel for the hit rate of this proposed cache. If the hit rate is not very high (75+%) the complications and overhead of the implementation may not be worthwhile.

[mpirvu]

Here's my proposed design (which still needs to be validated from performance point of view):
getMethodFromName() is essentially 2 frontend calls that are already handled correctly by JITServer:

TR_OpaqueMethodBlock *
TR_J9VM::getMethodFromName(const char *className, const char *methodName, const char *signature)
   {
   TR::VMAccessCriticalSection getMethodFromName(this);
   TR_OpaqueClassBlock *methodClass = getSystemClassFromClassName(className, strlen(className), true);
   TR_OpaqueMethodBlock * result = NULL;
   if (methodClass)
      result = (TR_OpaqueMethodBlock *)getMethodFromClass(methodClass, methodName, signature);

   return result;
   }

We can delete the JITServer implementation of getMethodFromName() and rely on those two frontend queries.
For getSystemClassFromClassName() the server already uses a cache (getClientData()->getClassBySignatureMap()).
For getMethodFromClass() we need to create another cache {j9class, methodName} --> j9method that will only hold methods belonging to classes loaded by the systemClassLoader. This will limit the number of classes/methods that are cached. If a class is not loaded by the systemClassLoader we will not attempt to use the cache, but just send a message to the client (can we determine the class loader of an arbitrary class? when these two frontend queries are used in pair we know that the classLoader is the systemClassLoader).
The cache {j9class, methodName} --> j9method has a few advantages:
(1) there is no possibility of duplication as when the key was just {classname, methodName}
(2) a j9class pointer takes less space than a string (className)
(3) Less overhead during entry deletion from the cache as explained below.

For deleting from the {j9class, methodName} --> j9method cache we still need to scan the entire cache for a matching j9class, but we no longer need to do string comparison. More importantly, we can avoid scanning if we know that the class being unloaded was not loaded by the systemClassLoader. In fact the systemClassLoader can never be unloaded, so in theory we could avoid scanning completely. However, system classes can still be redefined and redefinition is treated as unloading in the current implementation. Thus, we still need to check for potential deletion from our cache, but the number of such deletions should be very small (most of time it's going to be 0).

[KavinSatheeskumar]

This is the performance data for OpenJ9 on AcmeAirEE8 using jitserver without this change

Throughput stats: Avg= 1695.9  StdDev=  170.9  Min= 1360.1  Max= 1926.6  Max/Min=  42% CI95=    7.2% numSamples= 10
Footprint stats:  Avg=  234.8  StdDev=    3.7  Min=  227.0  Max=  240.4  Max/Min=   6% CI95=    1.1% numSamples= 10
Comp CPU stats:   Avg=   16.0  StdDev=    1.3  Min=   14.2  Max=   18.5  Max/Min=  30% CI95=    5.6% numSamples= 10
StartupTime stats:Avg= 6390.4  StdDev=  336.4  Min= 5677.0  Max= 6753.0  Max/Min=  19% CI95=    3.8% numSamples= 10

Message Stats, per compilation

Avg=61.0091867
StdDev=0.290402116
Min=60.59193
Max=61.566135
Max/Min=1.60781312

This is the performance data for OpenJ9 on AcmeAirEE8 using jitserver with this change

Throughput stats: Avg= 1697.7  StdDev=   99.1  Min= 1566.4  Max= 1810.3  Max/Min=  16% CI95=    4.2% numSamples= 10
Footprint stats:  Avg=  230.8  StdDev=    3.2  Min=  223.9  Max=  235.2  Max/Min=   5% CI95=    1.0% numSamples= 10
Comp CPU stats:   Avg=   15.9  StdDev=    1.2  Min=   14.6  Max=   18.3  Max/Min=  25% CI95=    5.7% numSamples=  9
StartupTime stats:Avg= 6108.3  StdDev=  266.7  Min= 5771.0  Max= 6497.0  Max/Min=  13% CI95=    3.1% numSamples= 1

Message Stats, per compilation

Avg=60.91660356
StdDev=0.332890288
Min=60.334534
Max=61.367367
Max/Min=1.711843834

It seems like there was a substantial improvement, especially to startup time

[mpirvu]

It seems like there was a substantial improvement, especially to startup time

When looking at performance you always have to look at the level of fluctuations. The 95% confidence interval for start-up time is 3.8% and 3.1%. This means that, statistically speaking, you cannot distinguish between 2 JVMs that are ~7% apart. You need a large number of runs to reduce that confidence interval.

My previous experiments showed about ~730 VM_getMethodFromName messages which are likely to disappear with your change (looking at AcmeAirEE8, warm run). The total number of messages is ~200,000. This means that this PR can cut about ~0.35% of total messages and the improvements (CompCPU especially) should be in the same ballpark.

[mpirvu]

In comment #23183 (comment) I was suggesting to use the new cache only for system classes. Given how TR_J9VM::getMethodFromName(const char *className, const char *methodName, const char *signature) is implemented (considers only system classes) , the hit rate of the new cache will be the same.
Then, TR_J9ServerVM::getMethodFromClass(TR_OpaqueClassBlock *methodClass, const char *methodName, const char *signature, TR_OpaqueClassBlock *callingClass) can change to do:

• search the <j9class, methodName> --> mapping (our cache)
• if entry found, return j9method
• send message to client and get the j9method
• search the _romClassMap for the j9class and retrieve ClassInfo and from there the classloader of j9class
• if classloader found and it is the system class loader, cache the <j9class, methodName> --> mapping
• return the j9method (either from cache or from the client)

Purging from cache can be simplified:

for each j9class to be unloaded/modified
   search `_romClassMap` and find the classloader of that j9class
   if the classloader is found and it is the system classloader
      scan the newly created cache

For the vast majority of the caches the linear scan of the newly created cache ca be avoided because system classes are never unloaded and they are rarely modified.

[mpirvu]

In ClientSessionData::processUnloadedClasses(const std::vector<TR_OpaqueClassBlock*> &classes, bool updateUnloadedClasses) we traverse the list of unloaded classes and for each such class we compute the classloader

 for (auto clazz : classes)
   {
   ...
    J9ClassLoader *cl = (J9ClassLoader *)(it->second._classLoader);
   }

At this point you can determine whether the classloader is the system classloader. If it is (unlikely) you should add this class to a locally defined vector.
Later on you should process the elements from this vector by scanning the newly added cache for a matching class. In the vast majority of the cases this new vector is going to be empty because system classes cannot be unloaded and they are rarely redefined.

[mpirvu]

I have some inline comments.

[mpirvu]

LGTM. I only have a small request for an extra comment/explanation.

[mpirvu]

It would be better to explain the meaning/purpose of the callingClass. This is explained in the base implementation.

 *     If callingClass is non-null, a visibility check will be done during the look up.
 *     Only methods visible to the callingClass will be returned.

The caching mechanism can be simplified if we use it only when callingClass == NULL.

[mpirvu]

The explanation in #23183 (comment) needs an update.

[mpirvu]

jenkins test sanity plinuxjit,xlinuxjit,zlinuxjit,alinux64jit jdk21

[mpirvu]

Failures:
aarch64: jdk_lang_j9_1, java/lang/Thread/virtual/Collectable.java timeout has been seen before: #18463

plinux: jdk_concurrent_1, java/util/concurrent/ArrayBlockingQueue/WhiteBox.java,
java.lang.AssertionError: failed to do a "full" gc
has been seen before in #19047

zlinux and zlinux: vector API failures. These are known