Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhanced the output with stats about JITClient specific Activity #9671

Merged
merged 1 commit into from
May 28, 2020

Conversation

EmanElsaban
Copy link
Contributor

Added how many compilations will be remote and how many messages of each specific type were sent
issue: #9339

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

@EmanElsaban EmanElsaban force-pushed the new-branch branch 3 times, most recently from a13258b to 451c367 Compare May 25, 2020 17:13
@EmanElsaban
Copy link
Contributor Author

@mpirvu Hi Marius Can you please review my PR? Thank you

@mpirvu mpirvu added the comp:jitserver Artifacts related to JIT-as-a-Service project label May 25, 2020
Copy link
Contributor

@mpirvu mpirvu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my inline comments

fprintf(stderr, "Level=%d\tnumComp=%d\n", i, _statsOptLevels[i]);
{
fprintf(stderr, "Level=%d\tnumComp=%d\tnumRemoteComp=%d\n", i, _statsOptLevels[i], _statsRemoteOptLevels[i]);
JITServerHelpers::printJITServerMsgStats(jitConfig);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code prints the same message stats a few times (for each opt level). Here we want stats about the remote compilations, not about messages.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please change JITServerHelpers::printJITServerMsgStats(); to display total number of messages and average number of messages per compilation.

Comment on lines 3126 to 3135
#if defined(J9VM_OPT_JITSERVER)
for (int i = 0; i < JITServer::MessageType_ARRAYSIZE; ++i)
{
if (JITServerHelpers::serverMsgTypeCount[i] > 0)
{
totalMsgCount += JITServerHelpers::serverMsgTypeCount[i];
}
}
#endif /* defined(J9VM_OPT_JITSERVER) */
fprintf(stderr, "Total Number of messages:%d\tAverage number of messages per compilation: %f \n", totalMsgCount, totalMsgCount/float(totalNumRemoteCompilations));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This piece of functionality is already done in HookedByTheJIT.cpp JitShutdown():

#if defined(J9VM_OPT_JITSERVER)
   static char * isPrintJITServerMsgStats = feGetEnv("TR_PrintJITServerMsgStats");
   if (isPrintJITServerMsgStats && compInfo->getPersistentInfo()->getRemoteCompilationMode() == JITServer::CLIENT)
      JITServerHelpers::printJITServerMsgStats(jitConfig);
   static char * isPrintJITServerCHTableStats = feGetEnv("TR_PrintJITServerCHTableStats");
   if (isPrintJITServerCHTableStats)
      JITServerHelpers::printJITServerCHTableStats(jitConfig, compInfo);
#endif

All you need to do is modify JITServerHelpers::printJITServerMsgStats so that it prints the number of total and average messages. The average can be computed by dividing the number of total messages to the number of messages of type compilationCode (every time a remote compilation occurs, the server will send a compilationCode message back at the end of the compilation.

}
j9tty_printf(PORTLIB, "Total number of messages:%d\tAverage number of messages per compilation:%f\n", totalMsgCount, totalMsgCount/float(JITServerHelpers::serverMsgTypeCount[0]));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a check that JITServerHelpers::serverMsgTypeCount[0] is not 0. Otherwise the JVM will crash when doing division by 0

if (_statsOptLevels[i] > 0)
fprintf(stderr, "Level=%d\tnumComp=%d\n", i, _statsOptLevels[i]);
fprintf(stderr, "Level=%d\tnumComp=%d\tnumRemoteComp=%d\n", i, _statsOptLevels[i], _statsRemoteOptLevels[i]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work on a build that does not have J9VM_OPT_JITSERVER defined because _statsRemoteOptLevels is going to be undefined.

Comment on lines 3123 to 3126
#if defined(J9VM_OPT_JITSERVER)
JITServerHelpers::printJITServerMsgStats(jitConfig);
#endif /* defined(J9VM_OPT_JITSERVER) */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't have to call JITServerHelpers::printJITServerMsgStats(jitConfig); because it's already called in jitShutdown where it prints stats about messages

Copy link
Contributor

@mpirvu mpirvu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please squash the two commits into one and maybe change the title of the commit to something like

"Print stats about JITClient specific activity at JVM shutdown"

Comment on lines 3119 to 3129
{
if (_statsOptLevels[i] > 0)
fprintf(stderr, "Level=%d\tnumComp=%d\n", i, _statsOptLevels[i]);
#if defined(J9VM_OPT_JITSERVER)
if (_statsRemoteOptLevels[i] > 0)
fprintf(stderr, "numRemoteComp=%d\n", _statsRemoteOptLevels[i]);
#endif /* defined(J9VM_OPT_JITSERVER) */

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow I think it looks nicer if the remote value appears on the same line, so maybe:

      for (int i = 0; i < (int)numHotnessLevels; i++)
         {
         if (_statsOptLevels[i] > 0)
            {
            fprintf(stderr, "Level=%d\tnumComp=%d", i, _statsOptLevels[i]);
#if defined(J9VM_OPT_JITSERVER)
            if (_statsRemoteOptLevels[i] > 0)
               fprintf(stderr, "\tnumRemoteComp=%d", _statsRemoteOptLevels[i]);
#endif /* defined(J9VM_OPT_JITSERVER) */
            fprintf(stderr, "\n");
            }
         }

Added how many compilations will be remote and how many messages were sent
printed the total number of messages and the average number of messages per compilation
issue: eclipse-openj9#9339

Signed-off-by: Eman Elsabban <eman.elsaban1@gmail.com>
Copy link
Contributor

@mpirvu mpirvu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mpirvu
Copy link
Contributor

mpirvu commented May 27, 2020

jenkins test sanity plinuxjit,xlinuxjit jdk11

1 similar comment
@mpirvu
Copy link
Contributor

mpirvu commented May 27, 2020

jenkins test sanity plinuxjit,xlinuxjit jdk11

@mpirvu
Copy link
Contributor

mpirvu commented May 27, 2020

jenkins test sanity xlinuxjit jdk11

@mpirvu mpirvu merged commit 4fa2976 into eclipse-openj9:master May 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:jitserver Artifacts related to JIT-as-a-Service project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants