Add information regarding synchronous compilations in javacore#24236
Add information regarding synchronous compilations in javacore#24236dev-koan wants to merge 4 commits into
Conversation
fbec998 to
2d33ba0
Compare
mpirvu
left a comment
There was a problem hiding this comment.
We have one decision to make: do we want to measure the times needed for synchronous compilations to be performed or the waiting time of application threads? there could be several application threads waiting for the same compilation to finish.
43f338e to
177844e
Compare
543fdd6 to
0825011
Compare
0825011 to
3ffce49
Compare
mpirvu
left a comment
There was a problem hiding this comment.
LGTM. Please post an example of the output.
|
Also, please squash the 4 commits into 1. |
3ffce49 to
75bbae1
Compare
Ideally, i'd like to see each app thread maintain its own longest wait time (and associated info). For brevity, only threads that have wait times longer than a certain threshold (like 3ms or 10ms) should be reported it to javacore. |
|
If each thread maintains the name of the sync method, with long method names and many threads there could be some footprint increase. I like the idea of applying some filtering and not include quick compilations. |
|
@amicic To clarify: each app thread that is subject to a wait due to a synchronous compilation will have a dynamically allocated string in J9VMThread struct (to hold the name of the method it waited for). This string needs to be freed when the thread is destroyed. |
75bbae1 to
286bba4
Compare
ee2ee8c to
1ca1fb1
Compare
mpirvu
left a comment
There was a problem hiding this comment.
LGTM. Please post an example of the output in the description of this PR.
| typedef struct J9JITSyncCompilationStatistics { | ||
| uint32_t totalCount; | ||
| uint64_t totalWaitTime; // microseconds (us) | ||
| J9JITLongestSyncComp longestWaitMethods[J9_LONGEST_SYNC_COMP]; |
There was a problem hiding this comment.
Please add a comment showing how this array is sorted.
1ca1fb1 to
a65babe
Compare
|
@amicic If you approve it, I am going to start testing and merge. Thanks |
| _OutputStream.writeCharacters("2nd Longest"); | ||
| } | ||
| else { | ||
| _OutputStream.writeCharacters("3rd Longest"); |
There was a problem hiding this comment.
Not quite the most robust (in case J9_LONGEST_SYNC_COMP increases a bit). you could simplify and just print numbers 1. longest, 2. longest
Just an observation. If you still prefer this way, i'm fine with it.
a65babe to
97d5371
Compare
Added a struct in J9JITConfig to store information regarding synchronous compilations and print them in javacore file. Signed-off-by: Dev Agarwal <dev.agarwal@ibm.com>
97d5371 to
88e217d
Compare
| for (int i = 0; i < J9_LONGEST_SYNC_COMP; i++) { | ||
| stats->longestWaitMethods[i].waitTime = 0; |
There was a problem hiding this comment.
This seems racy: other threads may be active.
Can we have a text version of that sample (as opposed to an image)? |
|
2376107 to
5052e90
Compare
| IDATA verboseOutputLevel; | ||
| omrthread_monitor_t compilationMonitor; | ||
| void* compilationInfo; | ||
| J9JITSyncCompilationStatistics *syncCompStats; |
There was a problem hiding this comment.
Why need this be indirect? Why not:
J9JITSyncCompilationStatistics syncCompStats;There was a problem hiding this comment.
It is simpler to use a pointer here to make sure the same memory is being accessed everywhere (in compilationthread class and javadump class)
I can also use a reference (&) to syncCompStats in javadump and compilationthread if that is preferred.
|
|
||
| struct CompilationStatistics _stats; | ||
| struct CompilationStatsPerInterval _intervalStats; | ||
| SyncCompilationStatistics _syncCompStats; |
There was a problem hiding this comment.
This should instead live in J9JITConfig.
| * Assisted-by: IBM Bob | ||
| *******************************************************************************/ | ||
|
|
||
| #include <cstdint> |
There was a problem hiding this comment.
I don't think this is necessary.
| _syncCompStats.totalWaitTime += duration; | ||
|
|
||
| /* Obtain index to insert. */ | ||
| int idx = -1; |
There was a problem hiding this comment.
This should use the same type as i.
| typedef struct J9JITSyncCompilationStatistics { | ||
| uint32_t totalCount; | ||
| uint64_t totalWaitTime; /* microseconds */ | ||
| J9JITLongestSyncComp longestWaitMethods[J9_NUM_LONGEST_SYNC_COMP]; /* sorted in increasing order */ |
There was a problem hiding this comment.
I think you mean "decreasing" (the first entry has the longest wait time).
| *******************************************************************************/ | ||
|
|
||
| /* Includes */ | ||
| #include <cstdint> |
There was a problem hiding this comment.
I don't think this is necessary.
| _OutputStream.writeCharacters("2JITTOTALWAIT Total application wait time: "); | ||
| _OutputStream.writeInteger64(stats->totalWaitTime, "%llu"); | ||
| _OutputStream.writeCharacters("us\n"); | ||
| for (int32_t i = 0; i < J9_NUM_LONGEST_SYNC_COMP; i++) { |
There was a problem hiding this comment.
Suggest using UDATA instead of int32_t, in part so the format on line 2953 is correct, and for consistency with other loops in this file.
|
|
||
| _OutputStream.writeCharacters("3JITENDTIME Wait time end: "); | ||
| omrstr_ftime_ex(timeStamp, _MaximumTimeStampLength, "%Y-%m-%dT%H:%M:%S", | ||
| longestWaitMethod.waitTimeEnd, OMRSTR_FTIME_FLAG_LOCAL); |
There was a problem hiding this comment.
Excessive indentation: Two tabs more than line 2961 is sufficient.

Added a struct in J9JITConfig to store information regarding synchronous compilations and print them in javacore file.
Signed-off-by: Dev Agarwal dev.agarwal@ibm.com