Add JFR CPULoad and ThreadCPU event support #20186

thallium · 2024-09-18T19:00:53Z

Depends on: eclipse/omr#7491

thallium · 2024-09-18T19:29:33Z

@tajila requesting your review

tajila · 2024-10-04T17:05:29Z

Im seeing negative numbers for Thread user mode CPU load

runtime/oti/j9nonbuilder.h

runtime/vm/jfr.cpp

runtime/oti/j9nonbuilder.h

runtime/vm/jfr.cpp

runtime/port/linux/j9thread.c

runtime/vm/jfr.cpp

This function returns the user and system cpu time of the calling thread. Related: eclipse-openj9/openj9#20186 Signed-off-by: Gengchen Tuo <[email protected]>

thallium · 2024-10-10T15:53:43Z

Opend OMR PR and addressed previous state.

This function returns the user and system cpu time of the calling thread. Related: eclipse-openj9/openj9#20186 Signed-off-by: Gengchen Tuo <[email protected]>

runtime/vm/jfr.cpp

keithc-ca · 2024-10-11T17:32:43Z

runtime/vm/jfr.cpp

+ if (count % 100 == 0) { // 1000ms
+ jfrCPULoad(currentThread);
+ }
+ if (count % 1000 == 0) { // 10s


Missing parentheses, constants go on the left of ==:

if (0 == (count % 100)) { // 1000ms

By the way, how do you conclude that 100 counts is 1000ms?

J9JFR_SAMPLING_RATE is defined to be 10 so 100 counts is 1000ms

This function returns the user and system cpu time of the calling thread. This new function is needed because the only way that we're aware of to get the user CPU time of a thread is to use getrusage(RUSAGE_THREAD, ...) which can only be called on the current thread. Related: eclipse-openj9/openj9#20186 Signed-off-by: Gengchen Tuo <[email protected]>

runtime/vm/JFRChunkWriter.hpp

Depends on: eclipse/omr#7491 Signed-off-by: Gengchen Tuo <[email protected]>

keithc-ca · 2024-10-11T20:09:13Z

runtime/vm/jfr.cpp

+ if (NULL != jfrEvent) {
+ initializeEventFields(currentThread, (J9JFREvent *)jfrEvent, J9JFR_EVENT_TYPE_CPU_LOAD);
+ UDATA numberOfCpus = j9sysinfo_get_number_CPUs_by_type(J9PORT_CPU_ONLINE);
+ U_64 currentTime = (U_64)j9time_nano_time();


This should not mix types: it should use int64_t as declared by OMR. Related locals should use int64_t instead of I_64.

Do you suggest changing prevProcTimestamp fields to int64_t as well?

Yes, they should use the same type as the source (int64_t omrtime_nano_time()).

This function returns the user and system cpu time of the calling thread. This new function is needed because the only way that we're aware of to get the user CPU time of a thread is to use getrusage(RUSAGE_THREAD, ...) which can only be called on the current thread. Related: eclipse-openj9/openj9#20186 Signed-off-by: Gengchen Tuo <[email protected]>

keithc-ca · 2024-10-11T20:26:29Z

runtime/vm/BufferWriter.hpp

+ if (_isLE) {
+ newVal = byteSwap(newVal);
+ }
+ writeData((U_8 *)&newVal, 4);


Suggest this be simplified to:

writeU32(newVal);

keithc-ca · 2024-10-11T20:53:51Z

runtime/oti/j9nonbuilder.h

@@ -5640,6 +5662,9 @@ typedef struct JFRState {
 void *constantEvents;
 BOOLEAN isConstantEventsInitialized;
 omrthread_monitor_t isConstantEventsInitializedMutex;
+ J9SysinfoCPUTime prevSysCPUTime;
+ omrthread_process_time_t prevProcCPUTime;


Suggest this field be named prevProcCPUTimes consistent with its source omrthread_get_process_times().

keithc-ca · 2024-10-11T20:58:02Z

runtime/vm/jfr.cpp

+ if (0 == (count % 1000)) { // 10s
+ J9SignalAsyncEvent(vm, NULL, vm->jfrThreadCPULoadAsyncKey);
+ }
+ count++;


This should use +=:

count += 1;

thallium force-pushed the cpuload branch from 1493fd1 to 6327e3d Compare September 18, 2024 19:16

tajila reviewed Oct 4, 2024

View reviewed changes

runtime/oti/j9nonbuilder.h Outdated Show resolved Hide resolved

tajila reviewed Oct 4, 2024

View reviewed changes