Stored the ram classes before sending the compilationReq to server #9955

EmanElsaban · 2020-06-19T15:44:43Z

Stored the romClasses in an unordered_map before sending the compilation
request to the server. I also check if the romClass is in the map, if it is
I send an empty string to the server instead of the romClass again and on the server
side I check to see if I received an empty string. If so I don't cache it.
issue: #9708

Signed-off-by: Eman Elsabban eman.elsaban1@gmail.com

EmanElsaban · 2020-06-19T16:56:41Z

@mpirvu Can you please review my PR? Thank you.

mpirvu

I have some inline comments

mpirvu · 2020-06-22T15:36:27Z

runtime/compiler/control/CompilationRuntime.hpp

@@ -1227,6 +1229,7 @@ class CompilationInfo

 #if defined(J9VM_OPT_JITSERVER)
   ClientSessionHT               *_clientSessionHT; // JITServer hashtable that holds session information about JITClients
+   PersistentUnorderedMap<J9ROMClass*, JITServerHelpers::ClassInfoTuple> _romJ9ClassMap;


Why do we need ClassInfoTuple? Maybe all we need here is an unordered_set instead of an unordered_map

Maybe add some comments with the purpose of this hashtable.

mpirvu · 2020-06-22T15:39:43Z

runtime/compiler/control/JITClientCompilationThread.cpp

@@ -3146,6 +3146,22 @@ remoteCompile(
   if (compiler->isOptServer())
      compiler->setOption(TR_Server);
   auto classInfoTuple = JITServerHelpers::packRemoteROMClassInfo(clazz, compiler->fej9vm()->vmThread(), compiler->trMemory());
+
+   // check if the romClass is in the map, if it is not insert in the map


I would prefer multi-line comments to be capitalized --> Check

mpirvu · 2020-06-22T15:42:14Z

runtime/compiler/control/JITClientCompilationThread.cpp

+      {
+      // send an empty string instead of the rom class   
+      // send_compile_request_without_romclass 
+      std::get<0>(classInfoTuple) = " ";


I wonder if we can optimize this path because we seem to serialize the ROMClass information and then throw it away

mpirvu · 2020-06-22T15:47:20Z

runtime/compiler/control/JITServerCompilationThread.cpp

@@ -528,8 +528,13 @@ TR::CompilationInfoPerThreadRemote::processEntry(TR_MethodToBeCompiled &entry, J
      J9ROMClass *romClass = NULL;
      if (!(romClass = JITServerHelpers::getRemoteROMClassIfCached(clientSession, clazz)))
         {
-         romClass = JITServerHelpers::romClassFromString(std::get<0>(classInfoTuple), compInfo->persistentMemory());
-         JITServerHelpers::cacheRemoteROMClass(getClientData(), clazz, romClass, &classInfoTuple);
+         //check whether the first argument of the classInfoTuple is an empty string


The the ROMClass is not already cached at the server we get the information from the request sent by the client and cache it. If the client did not send us anything, the server needs to specifically ask about the information, because it cannot continue with the ROMClas.

EmanElsaban · 2020-06-23T21:04:39Z

@dmitry-ten

mpirvu

I have a few observations. Also I need to think what should happen in the presence of class unloading.

mpirvu · 2020-06-24T01:09:13Z

runtime/compiler/control/JITClientCompilationThread.cpp

@@ -44,7 +44,7 @@
 #include "vmaccess.h"

 extern TR::Monitor *assumptionTableMutex;
-
+bool serializeClass = false;


This global can create problems with multithreading.
Please define a local as close as possible to the usage.

mpirvu · 2020-06-24T01:09:56Z

runtime/compiler/control/JITClientCompilationThread.cpp

@@ -1171,7 +1171,8 @@ handleServerMessage(JITServer::ClientStream *client, TR_J9VM *fe, JITServer::Mes
      case MessageType::ResolvedMethod_getRemoteROMClassAndMethods:
         {
         J9Class *clazz = std::get<0>(client->getRecvData<J9Class *>());
-         client->write(response, JITServerHelpers::packRemoteROMClassInfo(clazz, fe->vmThread(), trMemory));
+         serializeClass = true;
+         client->write(response, JITServerHelpers::packRemoteROMClassInfo(clazz, fe->vmThread(), trMemory, serializeClass));


Here you don't need serializeClass; instead you can pass directly true

runtime/compiler/control/JITClientCompilationThread.cpp

mpirvu · 2020-06-24T01:15:08Z

runtime/compiler/control/JITServerCompilationThread.cpp

@@ -298,6 +298,7 @@ TR::CompilationInfoPerThreadRemote::processEntry(TR_MethodToBeCompiled &entry, J
   bool abortCompilation = false;
   uint64_t clientId = 0;
   TR::CompilationInfo *compInfo = getCompilationInfo();
+   auto comp = getCompilation();


A comp object does not exist at this point. Most likely comp == NULL at this point

mpirvu · 2020-06-24T01:32:30Z

runtime/compiler/control/JITServerCompilationThread.cpp

+            // It could be a renewed connection, so that's a new server because old one was shutdown
+            // When the server receives an empty ROM class it would check if it actually has this class cached, 
+            // And if it it's not cached, send a request to the client 
+            romClass = JITServerHelpers::getRemoteROMClass(clazz, stream, comp->trMemory(), &classInfoTuple);


Here, because comp is likely NULL you cannot use comp->trMemory(). However, looking at the definition of getRemoteROMClass() the only reason it needs trMemory is for trMemory->trPersistentMemory() which you can obtain from compInfo->persistentMemory() as used above. So you need to modify getRemoteROMClass() to accept persistentMemory as a parameter.

mpirvu · 2020-06-24T01:34:19Z

runtime/compiler/control/JITServerCompilationThread.cpp

-         JITServerHelpers::cacheRemoteROMClass(getClientData(), clazz, romClass, &classInfoTuple);
+         // Check whether the first argument of the classInfoTuple is an empty string
+         // If it's an empty string then I dont't need to cache it 
+         if(std::get<0>(classInfoTuple) != " ")


This is not empty string, but rather a string with one character. You can actually create an empty string by calling the default constructor and test it with string.empty() function

mpirvu · 2020-06-24T01:38:07Z

runtime/compiler/control/JITServerHelpers.cpp

@@ -324,7 +333,7 @@ JITServerHelpers::packRemoteROMClassInfo(J9Class *clazz, J9VMThread *vmThread, T
   uintptr_t classChainOffsetOfIdentifyingLoaderForClazz = fe->sharedCache() ? 
      fe->sharedCache()->getClassChainOffsetOfIdentifyingLoaderForClazzInSharedCacheNoFail((TR_OpaqueClassBlock *)clazz) : 0;

-   return std::make_tuple(packROMClass(clazz->romClass, trMemory), methodsOfClass, baseClass, numDims, parentClass,
+   return std::make_tuple(packROMClass(clazz->romClass, trMemory, serializeClass), methodsOfClass, baseClass, numDims, parentClass,


You don't need to pass serializeClass to packROMClass. Instead you could do:
return std::make_tuple(serializeClass ? packROMClass(clazz->romClass, trMemory) : std::string(), methodsOfClass, baseClass, numDims, parentClass,

dmitry-ten · 2020-06-24T16:13:14Z

I have a concern that's similar to class unloading.
The current implementation will work correctly if the server goes down and another server comes up, i.e. server will detect that it doesn't have a ROM class cached and will send a message to the client.
However, this will result in some extra messages, since the new server doesn't have any ROM classes cached, so pretty much every compilation will result in 1 extra message. I'm not sure if there is an easy solution here. Server would basically need to inform the client that it doesn't have anything cached so that the client can clear its set.

@mpirvu I think we could just look at the list of unloaded classes at the client and delete them from the _romJ9ClassSet. We would need to find the corresponding ROM class from J9Class but that is easy to do.

mpirvu · 2020-06-24T17:42:38Z

I thought about deleting the RomClass from the local set at the client on class unloading. I guess I was trying to over-optimize because one ROMClass can be used by several RAMClasses (with different classloaders) and even if a RAMClass may die the ROMClass can still exist.
I now realized that the cache at the server is based on RAMClasses. Because the client keeps its cache based on ROMClasses it may not send the ROMClass for a second RAMClass that stems from the same ROMClass. This will result in message from the server to the client.
So, I propose that the unordered_set added at the client be based on RAMClasses. This set needs to be purged when a RAMClass gets unloaded.

mpirvu · 2020-06-29T21:04:07Z

In remoteCompile() the j9class for the method to be compiled is readily available: clazz. This is a better key for our set than the ROMClass.
That set needs to be purged when a class gets unloaded. When a class gets unloaded, static void jitHookClassUnload(J9HookInterface * * hookInterface, UDATA eventNum, void * eventData, void * userData) is called. In there we already have code JITServer code for adding to the unloadedClassesTemList:

#if defined(J9VM_OPT_JITSERVER)
   // Add to JITServer unload list
   if (compInfo->getPersistentInfo()->getRemoteCompilationMode() == JITServer::CLIENT)
      compInfo->getUnloadedClassesTempList()->push_back(clazz);
#endif

We just need to augment the code with some lines that would delete the corresponding j9class from our set. Since class unloading is done in a stop-the-world fashion (only one thread is active), acquiring a monitor is not needed.

mpirvu

I have some inline comments. Also, you need to increment the minor version for the CommunicationStream because we changed the content of the messages.

mpirvu · 2020-06-30T18:44:19Z

runtime/compiler/control/CompilationRuntime.hpp

@@ -1009,6 +1010,7 @@ class CompilationInfo
      }
   void setNewlyExtendedClasses(PersistentUnorderedMap<TR_OpaqueClassBlock*, uint8_t> *it) { _newlyExtendedClasses = it; }

+   TR::Monitor *getRamSetMonitor() const { return _ramSetMonitor; }


The name RamSet is not very descriptive.
I am not feeling very inspired, but maybe something along the lines classesCachedAtServerMonitor better depicts what this monitor is used for

mpirvu · 2020-06-30T18:46:55Z

runtime/compiler/control/HookedByTheJit.cpp

@@ -2115,6 +2115,12 @@ static void jitHookClassUnload(J9HookInterface * * hookInterface, UDATA eventNum
   // Add to JITServer unload list
   if (compInfo->getPersistentInfo()->getRemoteCompilationMode() == JITServer::CLIENT)
      compInfo->getUnloadedClassesTempList()->push_back(clazz);
+   // Loop through the set to find the class that needs to be purged once found erase from the set
+   auto it = compInfo->getRamJ9ClassSet().find(unloadedEvent->clazz);


This code should be put under the if that determines we are running on the client.

mpirvu · 2020-06-30T18:49:27Z

runtime/compiler/control/HookedByTheJit.cpp

+   auto it = compInfo->getRamJ9ClassSet().find(unloadedEvent->clazz);
+   if (it != compInfo->getRamJ9ClassSet().end())
+      { 
+      compInfo->getRamJ9ClassSet().erase(it);


You can try to erase it even if it does not exist, thus avoiding one search operation when desired item exists in the collection.

mpirvu · 2020-06-30T18:51:32Z

runtime/compiler/control/JITClientCompilationThread.cpp

+      OMR::CriticalSection romClassCache(compInfo->getRamSetMonitor());
+      if (compInfo->getRamJ9ClassSet().find(clazz) == compInfo->getRamJ9ClassSet().end())
+         {
+         // clazz was not found send the romclass


This comment is composed from two sentences. Let make that obvious: "clazz not found. Send the romClass to JITServer."

mpirvu · 2020-06-30T19:11:24Z

runtime/compiler/control/JITClientCompilationThread.cpp

+         {
+         // Send an empty string instead of the rom class, first don't serialize the ROMClass 
+         // Send_compile_request_without_romclass 
+         serializeClass = false;


The else part is not needed, because serializeClass starts as false

mpirvu · 2020-06-30T19:22:45Z

runtime/compiler/control/JITServerCompilationThread.cpp

+            // When the server receives an empty ROM class it would check if it actually has this class cached, 
+            // And if it it's not cached, send a request to the client 
+            romClass = JITServerHelpers::getRemoteROMClass(clazz, stream, compInfo->persistentMemory(), &classInfoTuple);
+            JITServerHelpers::cacheRemoteROMClass(getClientData(), clazz, romClass, &classInfoTuple);


The line JITServerHelpers::cacheRemoteROMClass(getClientData(), clazz, romClass, &classInfoTuple); is common for if and else part so it can be taken out and placed after the if-then-else

mpirvu · 2020-06-30T19:27:52Z

runtime/compiler/control/JITServerHelpers.cpp

@@ -350,6 +350,15 @@ JITServerHelpers::getRemoteROMClass(J9Class *clazz, JITServer::ServerStream *str
   return romClassFromString(std::get<0>(*classInfoTuple), trMemory->trPersistentMemory());
   }

+J9ROMClass *


Is the previous version of JITServerHelpers::getRemoteClass() needed anymore? If not, we can delete it.

yes, it's used in getAndCacheRemoteROMClass() in CompilationThread.cpp and getAndCacheRemoteROMClass is used in other several files. That's why I overloaded the function. For example, here _romClass = threadCompInfo->getAndCacheRemoteROMClass(_ramClass, trMemory); which is in j9methodServer.cpp.

mpirvu · 2020-07-02T14:54:18Z

runtime/compiler/control/CompilationRuntime.hpp

@@ -1227,6 +1230,8 @@ class CompilationInfo

 #if defined(J9VM_OPT_JITSERVER)
   ClientSessionHT               *_clientSessionHT; // JITServer hashtable that holds session information about JITClients
+   PersistentUnorderedSet<J9Class*> _ramJ9ClassSet;


Similar comment here about the _ramJ9ClassSet. Maybe replace with _classesCachedAtServer

mpirvu · 2020-07-02T14:55:48Z

runtime/compiler/control/HookedByTheJit.cpp

@@ -2115,6 +2115,12 @@ static void jitHookClassUnload(J9HookInterface * * hookInterface, UDATA eventNum
   // Add to JITServer unload list
   if (compInfo->getPersistentInfo()->getRemoteCompilationMode() == JITServer::CLIENT)
      compInfo->getUnloadedClassesTempList()->push_back(clazz);
+   // Loop through the set to find the class that needs to be purged once found erase from the set


The comment seems to be composed of two sentences, so let's make that clear:

// Loop through the set to find the class that needs to be purged. // Once found erase from the set.

mpirvu · 2020-07-02T15:00:35Z

runtime/compiler/control/JITClientCompilationThread.cpp

@@ -3145,7 +3144,28 @@ remoteCompile(

   if (compiler->isOptServer())
      compiler->setOption(TR_Server);
-   auto classInfoTuple = JITServerHelpers::packRemoteROMClassInfo(clazz, compiler->fej9vm()->vmThread(), compiler->trMemory());
+
+      // Check if the Clazz is in the set, if it is not insert in the set


Transform this comment into:

Check the _classesCachedAtServer set to determine whether JITServer is likely to have this class already cached. If so, do not send the ROMClass content to save network traffic."

mpirvu

LGTM

mpirvu · 2020-07-06T18:49:00Z

jenkins test sanity plinuxjit,xlinuxjit jdk11

mpirvu · 2020-07-06T18:53:17Z

It just occurred to me that in cases where the server clears its internal caches because of a seqNo that is lost we must destroy the set of cached j9classes at the client as well.
The easiest way is to accomplish this is to clear it when the client receives JITServer::MessageType::getUnloadedClassRangesAndCHTable

mpirvu · 2020-07-06T23:13:44Z

runtime/compiler/control/JITClientCompilationThread.cpp

@@ -215,6 +215,7 @@ handleServerMessage(JITServer::ClientStream *client, TR_J9VM *fe, JITServer::Mes
         auto table = (JITClientPersistentCHTable*)comp->getPersistentInfo()->getPersistentCHTable();
         std::string encoded = FlatPersistentClassInfo::serializeHierarchy(table);

+         compInfo->getclassesCachedAtServer().clear();


This require a mutex.
Maybe it's better to put is after the write operation so that we don't delay it.

mpirvu · 2020-07-07T12:55:48Z

Also, you need to increment the minor version for the CommunicationStream because we changed the content of the messages.

I realized that this is missing, so please increment the minor version.

Stored the ramClasses in an unordered_set before sending the compilation request to the server. I check if the ram class (clazz) is cached in the set. if it is not stored then it will be cached in the set and will send the corresponding ROM by setting the serializeClass bool to true. If the clazz is already in the cache set this means it has already been sent to the server. So send the clazz again but with empty ROM class (empty string for ROM Class) by setting serializeClass bool to false. issue: eclipse-openj9#9708 Signed-off-by: Eman Elsabban <eman.elsaban1@gmail.com>

mpirvu

LGTM

mpirvu · 2020-07-09T15:15:18Z

jenkins test sanity plinuxjit,xlinuxjit jdk11

mpirvu added the comp:jitserver Artifacts related to JIT-as-a-Service project label Jun 19, 2020

mpirvu added this to In progress in JIT as a Service via automation Jun 19, 2020

mpirvu self-assigned this Jun 19, 2020

mpirvu requested changes Jun 22, 2020

View reviewed changes

EmanElsaban force-pushed the romClassCache branch from 16940d8 to 7024052 Compare June 23, 2020 17:32

mpirvu requested changes Jun 24, 2020

View reviewed changes

EmanElsaban force-pushed the romClassCache branch 2 times, most recently from 7c09331 to cb6c0f4 Compare June 30, 2020 16:58

EmanElsaban changed the title ~~Stored the RomClasses before sending the compilationReq to server~~ Stored the ram classes before sending the compilationReq to server Jun 30, 2020

mpirvu reviewed Jul 2, 2020

View reviewed changes

EmanElsaban force-pushed the romClassCache branch from cb6c0f4 to ab8f015 Compare July 3, 2020 18:52

mpirvu approved these changes Jul 6, 2020

View reviewed changes

EmanElsaban force-pushed the romClassCache branch from ab8f015 to be21e04 Compare July 6, 2020 21:46

mpirvu reviewed Jul 6, 2020

View reviewed changes

EmanElsaban force-pushed the romClassCache branch from be21e04 to 3e7486a Compare July 7, 2020 12:35

EmanElsaban force-pushed the romClassCache branch from 3e7486a to 87efa9e Compare July 7, 2020 14:09

mpirvu approved these changes Jul 9, 2020

View reviewed changes

mpirvu merged commit a105721 into eclipse-openj9:master Jul 11, 2020

JIT as a Service automation moved this from In progress to Done Jul 11, 2020

dmitry-ten mentioned this pull request Jul 27, 2020

testJITServer_0 crash vmState=0x0005ffff #10215

Closed

Stored the ram classes before sending the compilationReq to server #9955

Stored the ram classes before sending the compilationReq to server #9955

Conversation

EmanElsaban commented Jun 19, 2020

EmanElsaban commented Jun 19, 2020

mpirvu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmanElsaban commented Jun 23, 2020

mpirvu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitry-ten commented Jun 24, 2020

mpirvu commented Jun 24, 2020

mpirvu commented Jun 29, 2020

mpirvu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpirvu left a comment

Choose a reason for hiding this comment

mpirvu commented Jul 6, 2020

mpirvu commented Jul 6, 2020

Choose a reason for hiding this comment

mpirvu commented Jul 7, 2020

mpirvu left a comment

Choose a reason for hiding this comment

mpirvu commented Jul 9, 2020