Client Request Cached Methods List from JIT Server #20129

luke-li-2003 · 2024-09-06T20:44:16Z

Added an option that makes the client request a list of cached methods from the server on bootstrap, and set the count for those methods as scount to improve rampup

Depends on eclipse/omr#7458

luke-li-2003 · 2024-09-06T20:44:42Z

@mpirvu

marius-pirvu

I provided a partial review of the code.

marius-pirvu · 2024-09-14T19:46:25Z

runtime/compiler/net/StreamExceptions.hpp

+      return _cacheName;
+      }
+private:
+   std::string _cacheName;


This could be marked const because nobody should be modifying it.

marius-pirvu · 2024-09-14T19:47:33Z

runtime/compiler/net/StreamExceptions.hpp

+      {
+      return "Requesting AOT cache content";
+      }
+   std::string getCacheName() const


This could return a const reference to the embedded string

marius-pirvu · 2024-09-14T19:48:55Z

runtime/compiler/net/StreamExceptions.hpp

+class StreamAotCacheMapRequest: public virtual std::exception
+   {
+public:
+   StreamAotCacheMapRequest(std::string cacheName) : _cacheName(cacheName)


Not sure if it's better to use a const reference for the parameter.

marius-pirvu · 2024-09-14T19:50:45Z

runtime/oti/j9nonbuilder.h

@@ -6159,6 +6159,9 @@ typedef struct J9JavaVM {
 	UDATA closeScopeNotifyCount;
 #endif /* JAVA_SPEC_VERSION >= 22 */
 	UDATA unsafeIndexableHeaderSize;
+


I would avoid adding new fields to the JavaVM struct. If only the JIT needs them, then J9JITConfig is a better place. The new fields are specific to JITServer so they need to be protected with #if defined(J9VM_OPT_JITSERVER)

marius-pirvu · 2024-09-14T20:02:29Z

runtime/compiler/control/rossa.cpp

+      return rc;
+   }
+
+   JITServer::ClientStream *client =


If the server is not available, this will throw an exception which needs to be caught.

marius-pirvu · 2024-09-16T09:12:45Z

runtime/compiler/control/JITServerCompilationThread.cpp

@@ -1101,6 +1109,43 @@ TR::CompilationInfoPerThreadRemote::processEntry(TR_MethodToBeCompiled &entry, J
      stream->writeError(compilationLowPhysicalMemory, (uint64_t) computeServerMemoryState(getCompilationInfo()));
      abortCompilation = true;
      }
+   catch (const JITServer::StreamAotCacheMapRequest &e)
+      {
+      std::string aotCacheName = e.getCacheName();


You can return just a const reference to the internal cache name.

marius-pirvu · 2024-09-16T09:13:47Z

runtime/compiler/control/JITServerCompilationThread.cpp

+      if (TR::Options::getVerboseOption(TR_VerboseJITServer))
+         {
+         TR_VerboseLog::writeLineLocked(TR_Vlog_JITServer,
+            "compThreadID=%d handling request for AOT cache list %s",


"handling request for AOT cache list %s" --> "handling request for AOT cache %s method list"

marius-pirvu · 2024-09-16T09:18:56Z

runtime/compiler/control/JITServerCompilationThread.cpp

+      auto cachedAOTMethod = aotCache->getCachedMethodHead();
+
+      std::vector<std::string> methodSignaturesV;
+      for (;cachedAOTMethod != NULL;


I think we need to acquire the lock that protects the map/list of methods.
We also need to account for possible exceptions (like not enough memory to allocate)

marius-pirvu · 2024-09-16T10:13:11Z

runtime/compiler/control/JITServerCompilationThread.cpp

+      auto aotCache = aotCacheMap->get(aotCacheName, 0, pending);
+      auto cachedAOTMethod = aotCache->getCachedMethodHead();
+
+      std::vector<std::string> methodSignaturesV;


When you know the number of elements in the vector you are building, it's better to size the vector to the right length, to avoid multiple resize operations later on which involves copying object.

marius-pirvu · 2024-09-16T10:36:08Z

runtime/compiler/control/JITServerCompilationThread.cpp

+           cachedAOTMethod = cachedAOTMethod->getNextRecord())
+         {
+         const SerializedAOTMethod &serializedAOTMethod = cachedAOTMethod->data();
+         std::string sig = std::string(serializedAOTMethod.signature());


Theoretically, the code can throw bad_alloc so we need another try region.

luke-li-2003 · 2024-09-17T14:28:36Z

Forgot to force-push, should be fine now

marius-pirvu · 2024-09-17T18:21:15Z

runtime/compiler/control/HookedByTheJit.cpp

+      int32_t sigLen = J9UTF8_LENGTH(className) + J9UTF8_LENGTH(name) +
+         J9UTF8_LENGTH(signature) + 2;
+
+      char *sigC = new char[sizeof(char) * (sigLen + 1)];


OpenJ9 has its own memory management that provides accounting, so using the system allocator is not a good choice. On the other hand we want to minimize the usage of persistent allocator because of the overhead and fragmentation it can create. Here's an example on how this achieved in other parts of the code: https://github.com/eclipse-openj9/openj9/blob/master/runtime/compiler/control/HookedByTheJit.cpp#L180
Basically we use the stack for most methods, and for the few very long signatures we use the persistent allocator. In this case I think that we can not set a low count for methods that have a very long name. These should be very few.

marius-pirvu · 2024-09-17T18:29:08Z

runtime/compiler/control/HookedByTheJit.cpp

+
+      // contains
+      methodCachedAtServer =
+         (serverAOTMethodSet->find(std::string(sigC)) == serverAOTMethodSet->end());


I think that == needs to be changed to !=

marius-pirvu · 2024-09-17T18:34:28Z

runtime/compiler/control/HookedByTheJit.cpp

+#if defined(J9VM_OPT_JITSERVER)
+      else if (methodCachedAtServer) // Higher priority than SCC
+         {
+         int32_t scount = optionsAOT->getInitialSCount();


Please check: if local shared classes is not used, optionsAOT may not exist.

You may use countInOptionSet || TR::Options::getCountsAreProvidedByUser() to check whether the use has specified counts.

marius-pirvu · 2024-09-17T18:44:28Z

runtime/compiler/control/JITServerCompilationThread.cpp

@@ -1101,6 +1108,58 @@ TR::CompilationInfoPerThreadRemote::processEntry(TR_MethodToBeCompiled &entry, J
      stream->writeError(compilationLowPhysicalMemory, (uint64_t) computeServerMemoryState(getCompilationInfo()));
      abortCompilation = true;
      }
+   catch (const JITServer::StreamAotCacheMapRequest &e)
+      {
+      const std::string aotCacheName = e.getCacheName();


Let's make this a reference since that's what getCacheName() returns.

marius-pirvu

Since the IDs of the various network messages has changed, we need to increment the minor version of the server.
Look how we do that in PR #19477 in file runtime/compiler/net/CommunicationStream.hpp

marius-pirvu · 2024-09-19T14:58:58Z

runtime/compiler/control/HookedByTheJit.cpp

+         methodCachedAtServer =
+            (serverAOTMethodSet->find(std::string(sigC)) != serverAOTMethodSet->end());
+
+         if (TR::Options::getVerboseOption(TR_VerboseJITServer))


There are too many of these messages that will be printed. I suggest we do the printing when both TR_VerboseJITServer and TR_VerboseCounts are enabled, since this code relates to setting counts.

marius-pirvu · 2024-09-19T15:07:23Z

runtime/compiler/control/HookedByTheJit.cpp

@@ -475,6 +515,24 @@ static void jitHookInitializeSendTarget(J9HookInterface * * hook, UDATA eventNum
                     TR::Options::getHighCodeCacheOccupancyBCount() :
                     TR::Options::getHighCodeCacheOccupancyCount();
         }
+#if defined(J9VM_OPT_JITSERVER)
+      else if (methodCachedAtServer) // Higher priority than SCC


I think that this code should not have higher priority than the case when a method is in local SCC, because the JIT will load the method from local SCC if available (it's just faster than the remote AOT cache)

marius-pirvu · 2024-09-19T15:23:23Z

runtime/compiler/control/JITServerCompilationThread.cpp

+
+      auto aotCacheMap = compInfo->getJITServerAOTCacheMap();
+      bool pending = false;
+      auto aotCache = aotCacheMap->get(aotCacheName, 0, pending);


Ask Christian about the interaction of this code with AOT cache persistence.
@param pending Output flag indicating that a cache load operation from file is in progress, but not read yet
Can we just ignore the pending parameter?

marius-pirvu · 2024-09-19T15:28:28Z

runtime/compiler/control/JITServerCompilationThread.cpp

+               {
+               const SerializedAOTMethod &serializedAOTMethod = cachedAOTMethod->data();
+               std::string sig = std::string(serializedAOTMethod.signature());
+               methodSignaturesV.push_back(sig);


Suggestion
methodSignaturesV.push_back(std::string(serializedAOTMethod.signature());:

marius-pirvu · 2024-09-19T15:41:00Z

runtime/compiler/control/JITServerCompilationThread.cpp

+         {
+            {
+            OMR::CriticalSection cs(cachedMethodMonitor);
+


Suggestion:

size_t numAOTMethods = _cachedMethodMap.size(); // Get number of methods in AOT cache (needs an accessor) methodSignaturesV.reserve(numAOTMethods);

Reading the size of the map needs to be done after we obtain the lock.

marius-pirvu · 2024-09-19T18:11:02Z

runtime/compiler/control/HookedByTheJit.cpp

@@ -4181,6 +4239,9 @@ void JitShutdown(J9JITConfig * jitConfig)

   TR::CompilationInfo * compInfo = TR::CompilationInfo::get(jitConfig);

+   if (jitConfig->serverAOTMethodSet)


We need to destruct the set. Calling just jitPersistentFree will only deallocate the memory used for the set itself, but not the entries added to the set.

marius-pirvu · 2024-09-23T21:09:53Z

runtime/compiler/control/JITServerCompilationThread.cpp

+         catch (const std::bad_alloc &e)
+            {
+            if (TR::Options::isAnyVerboseOptionSet(TR_VerboseJITServer,
+                                                   TR_VerboseCompilationDispatch))


I don't see a reason for printing a message into vlog under TR_VerboseCompilationDispatch

marius-pirvu · 2024-09-23T21:57:49Z

runtime/compiler/control/HookedByTheJit.cpp

@@ -628,6 +669,24 @@ static void jitHookInitializeSendTarget(J9HookInterface * * hook, UDATA eventNum
            }
 #endif // defined(J9VM_INTERP_AOT_COMPILE_SUPPORT) && defined(J9VM_OPT_SHARED_CLASSES) && (defined(TR_HOST_X86) || defined(TR_HOST_POWER) || defined(TR_HOST_S390) || defined(TR_HOST_ARM) || defined(TR_HOST_ARM64))
         } // if (TR::Options::sharedClassCache())
+#if defined(J9VM_OPT_JITSERVER)
+      else if (methodCachedAtServer) // Not cached in SCC but the server


We would like to set a low count either when SCC/AOT is not set, or when it is set but the method is not in local SCC.
We may have to use the code below in more than one place. Maybe define it in a function?

Added an option that makes the client request a list of cached methods from the server on bootstrap, and set the count for those methods as scount to improve rampup Signed-off-by: Luke Li <luke.li@ibm.com>

luke-li-2003 requested a review from dsouzai as a code owner September 6, 2024 20:44

mpirvu added the comp:jitserver Artifacts related to JIT-as-a-Service project label Sep 6, 2024

marius-pirvu reviewed Sep 16, 2024

View reviewed changes

luke-li-2003 force-pushed the GetServerAOTCacheList branch 4 times, most recently from 1796628 to 0952284 Compare September 17, 2024 14:27

luke-li-2003 force-pushed the GetServerAOTCacheList branch 3 times, most recently from dcc00d3 to cf589c4 Compare September 17, 2024 15:31

marius-pirvu reviewed Sep 17, 2024

View reviewed changes

luke-li-2003 force-pushed the GetServerAOTCacheList branch from cf589c4 to be50356 Compare September 18, 2024 13:38

marius-pirvu reviewed Sep 19, 2024

View reviewed changes

dsouzai added the depends:omr label Sep 19, 2024

marius-pirvu reviewed Sep 19, 2024

View reviewed changes

luke-li-2003 force-pushed the GetServerAOTCacheList branch 6 times, most recently from 2fa5254 to 4f059f1 Compare September 23, 2024 16:20

marius-pirvu reviewed Sep 23, 2024

View reviewed changes

luke-li-2003 force-pushed the GetServerAOTCacheList branch from 4f059f1 to 4b9803d Compare September 23, 2024 21:30

marius-pirvu reviewed Sep 23, 2024

View reviewed changes

Client Request Cached Methods List from JIT Server

adfa1aa

Added an option that makes the client request a list of cached methods from the server on bootstrap, and set the count for those methods as scount to improve rampup Signed-off-by: Luke Li <luke.li@ibm.com>

luke-li-2003 force-pushed the GetServerAOTCacheList branch from 4b9803d to adfa1aa Compare September 24, 2024 13:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client Request Cached Methods List from JIT Server #20129

Client Request Cached Methods List from JIT Server #20129

luke-li-2003 commented Sep 6, 2024 •

edited by dsouzai

Loading

luke-li-2003 commented Sep 6, 2024

marius-pirvu left a comment

marius-pirvu Sep 14, 2024

marius-pirvu Sep 14, 2024

marius-pirvu Sep 14, 2024

marius-pirvu Sep 14, 2024

marius-pirvu Sep 14, 2024

marius-pirvu Sep 16, 2024

marius-pirvu Sep 16, 2024

marius-pirvu Sep 16, 2024

marius-pirvu Sep 16, 2024

marius-pirvu Sep 16, 2024

luke-li-2003 commented Sep 17, 2024

marius-pirvu Sep 17, 2024

marius-pirvu Sep 17, 2024

marius-pirvu Sep 17, 2024

marius-pirvu Sep 17, 2024

marius-pirvu Sep 17, 2024

marius-pirvu left a comment •

edited

Loading

marius-pirvu Sep 19, 2024

marius-pirvu Sep 19, 2024

marius-pirvu Sep 19, 2024

marius-pirvu Sep 19, 2024

marius-pirvu Sep 19, 2024

marius-pirvu Sep 23, 2024

marius-pirvu Sep 19, 2024

marius-pirvu Sep 23, 2024

marius-pirvu Sep 23, 2024

		@@ -4181,6 +4239,9 @@ void JitShutdown(J9JITConfig * jitConfig)

		TR::CompilationInfo * compInfo = TR::CompilationInfo::get(jitConfig);

		if (jitConfig->serverAOTMethodSet)

Client Request Cached Methods List from JIT Server #20129

Are you sure you want to change the base?

Client Request Cached Methods List from JIT Server #20129

Conversation

luke-li-2003 commented Sep 6, 2024 • edited by dsouzai Loading

luke-li-2003 commented Sep 6, 2024

marius-pirvu 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

luke-li-2003 commented Sep 17, 2024

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

marius-pirvu left a comment • edited Loading

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

luke-li-2003 commented Sep 6, 2024 •

edited by dsouzai

Loading

marius-pirvu left a comment •

edited

Loading