Add JFR ThreadContextSwitchRate support #20725
Conversation
|
@tajila FYI |
thallium
force-pushed
the
jfr-context-switch
branch
3 times, most recently
from
5a1927f to
79fbfc8
Compare
|
@gengchen please rebase these changes |
|
@theresa-m please review these changes |
thallium
force-pushed
the
jfr-context-switch
branch
2 times, most recently
from
921b010 to
1f112d9
Compare
runtime/vm/JFRConstantPoolTypes.cpp
Outdated
| entry->switchRate = threadContextSwitchRateData->switchRate; | ||
|
|
||
| index = _threadContextSwitchRateCount; | ||
| _threadContextSwitchRateCount += 1; | ||
|
|
||
| done: | ||
| return index; |
There was a problem hiding this comment.
Why does this method return a value? The result is not used.
There was a problem hiding this comment.
This is just to be consistent with the initial JFR code in case we need the result in the future.
There was a problem hiding this comment.
This is just to be consistent with the initial JFR code in case we need the result in the future.
The old design required the return value for all events but this is no longer needed. You can remove it from this function and we can update the others in a separate PR.
runtime/vm/jfr.cpp
Outdated
| PORT_ACCESS_FROM_VMC(currentThread); | ||
| OMRPORT_ACCESS_FROM_J9PORT(PORTLIB); | ||
|
|
||
| uint64_t switches; |
There was a problem hiding this comment.
Please declare variables as the first line after the if statement.
There was a problem hiding this comment.
I don't see a problem here?
There was a problem hiding this comment.
Generally in vm code variable declarations go at the top to support old compilers. I see both patterns in this file so I'm not sure if that applies to cpp @tajila ?
There was a problem hiding this comment.
Do you mean that the switches should be on top of port access?
There was a problem hiding this comment.
Generally in vm code variable declarations go at the top to support old compilers. I see both patterns in this file so I'm not sure if that applies to cpp
This does not apply to C++ files. Due to legacy C rules (C89 and before), certain platforms will emit a warning (and error out because of warnings-as-errors) if C code has blocks wherein there are executable statements before variable declarations. From C99 and on, this was no longer a rule, but a convention for C code.
In C++, on the other hand, it is encouraged to declare variables close to the first use; there is no restriction/danger to declaring them anywhere in the block, provided they are declared before they are used of course.
Some VM C++ code looks like C code because it once was. But we don't need to restrict ourselves to legacy C standards going forward with C++.
All that being said, this is still technically valid C since the macros PORT_ACCESS_FROM_VMC and OMRPORT_ACCESS_FROM_J9PORT expand to variable declarations anyways.
However, we should be initializing all variables with an explicit value, which is missing from this line.
See https://sap.github.io/SapMachine/jfrevents/21.html#threadcontextswitchrate, basically it outputs the rate of context switches in the OS. |
thallium
force-pushed
the
jfr-context-switch
branch
2 times, most recently
from
162aafd to
389cd85
Compare
runtime/vm/jfr.cpp
Outdated
| PORT_ACCESS_FROM_VMC(currentThread); | ||
| OMRPORT_ACCESS_FROM_J9PORT(PORTLIB); | ||
|
|
||
| uint64_t switches; |
There was a problem hiding this comment.
Generally in vm code variable declarations go at the top to support old compilers. I see both patterns in this file so I'm not sure if that applies to cpp @tajila ?
thallium
force-pushed
the
jfr-context-switch
branch
from
389cd85 to
99788aa
Compare
Depends on: eclipse-omr/omr#7580 Signed-off-by: Gengchen Tuo <[email protected]>
thallium
force-pushed
the
jfr-context-switch
branch
from
99788aa to
b15f557
Compare
|
jenkins test sanity xlinux jdk17 |
|
jenkins test sanity alinux64 jdk21 |
|
jenkins compile win jdk8 |
| @@ -719,6 +722,7 @@ initializeJFR(J9JavaVM *vm, BOOLEAN lateInit) | |||
|
|
|||
| vm->jfrState.prevSysCPUTime.timestamp = -1; | |||
| vm->jfrState.prevProcTimestamp = -1; | |||
| vm->jfrState.prevContextSwitchTimestamp = -1; | |||
There was a problem hiding this comment.
This should explicitly initialize prevContextSwitches (or have a comment explaining why that's not necessary).
| if (-1 == jfrState->prevContextSwitchTimestamp) { | ||
| jfrEvent->switchRate = 0; | ||
| } else { | ||
| int64_t timeDelta = currentTime - jfrState->prevContextSwitchTimestamp; | ||
| jfrEvent->switchRate = ((double)(switches - jfrState->prevContextSwitches) / timeDelta) * 1e9; | ||
| } |
There was a problem hiding this comment.
This should either use float or have an explicit cast to float (the type of switchRate).
- explicitly initialize prevContextSwitches - calculate switchRate with float Related: eclipse-openj9#20725 Signed-off-by: Gengchen Tuo <[email protected]>
- explicitly initialize prevContextSwitches - calculate switchRate with float Related: eclipse-openj9#20725 Signed-off-by: Gengchen Tuo <[email protected]>
- explicitly initialize prevContextSwitches - calculate switchRate with float Related: eclipse-openj9#20725 Signed-off-by: Gengchen Tuo <[email protected]>
- explicitly initialize prevContextSwitches - calculate switchRate with float Related: eclipse-openj9#20725 Signed-off-by: Gengchen Tuo <[email protected]>
Depends on: eclipse-omr/omr#7580