add a caching mechanism for the getMethodFromName function on the VMJ9Server

[KavinSatheeskumar]

In order to reduce the messages sent between the JITServer and Client, we introduce a caching mechanism for calls fe->getMethodFromName(...) as overloaded by VMJ9Server.cpp

[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. Add a hash map (with a corresponding monitor) to the ClientSessionData structure which is used to cache calls to getMethodFromName
   • the keys is a triple (implemented as std::pair<std::pair<...,...>,...>) of the arguments to getMethodFromName
     • the first string is the method name, the second string is the signature, and the final string (the one that isn't nested) is the class name
     • the reason we don't use tuple<...> is because it is not hashable by default, so we would need to provide a hash function.
   • the value is a TR_OpaqueMethodBlock* which is the cached output of getMethodFromName
2. When the JITServer calls getMethodFromName it first
   • checks the cache to see if the method already exists
   • if not, sends a message to the client
   • if the value returned by the client is not NULL, then it caches the method
3. When a class is unloaded/redefined
   • Iterate through the map and filter all methods whose class name corresponds to the unloaded class

[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]

This should be a const reference to avoid creating another string. Even the triplet tpl can be a const reference.

[mpirvu]

You could use memcmp which may be optimized internally.

[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).