Do not build class verification exit tracepoint by default #21104
Conversation
Adds a J9VM_OPT_INCLUDE_CLASSVERIFY_EXIT_TRACEPOINT macro (not defined by default) to guard the recently (re?)introduced VerifyExit tracepoint, which resulted in some start-up regressions in -Xint mode. Signed-off-by: Mark Stoodley <[email protected]>
|
jenkins compile amac jdk11 |
runtime/vm/ClassInitialization.cpp
Outdated
| @@ -271,7 +271,10 @@ performVerification(J9VMThread *currentThread, J9Class *clazz) | |||
| } | |||
| } | |||
| done: | |||
| #if defined(J9VM_OPT_INCLUDE_CLASSVERIFY_EXIT_TRACEPOINT) | |||
| /* This tracepoint can introduce startup regressions even when disabled, so don't build into the VM by default */ | |||
There was a problem hiding this comment.
Please fold this line and finish the sentence with a period.
There was a problem hiding this comment.
Sorry, what do you mean by "fold this line" ? Do you mean put it on the same line as the #if defined()... ?
There was a problem hiding this comment.
Maybe "split" would have been a better word than "fold" (it's too long if you can't see all of it without scrolling horizontally here).
runtime/vm/ClassInitialization.cpp
Outdated
| Trc_VM_performVerification_Exit(currentThread); | ||
| #endif |
There was a problem hiding this comment.
This is missing the comment connecting it with line 274.
runtime/vm/ClassInitialization.cpp
Outdated
| @@ -271,7 +271,10 @@ performVerification(J9VMThread *currentThread, J9Class *clazz) | |||
| } | |||
| } | |||
| done: | |||
| #if defined(J9VM_OPT_INCLUDE_CLASSVERIFY_EXIT_TRACEPOINT) | |||
There was a problem hiding this comment.
What is the proposed mechanism to enable this tracepoint?
The corresponding entry tracepoint should be also be conditional so entry/exit tracing is balanced (and nesting is properly displayed when using -Xtrace:iprint...).
There was a problem hiding this comment.
I was thinking we could make the entry tracepoint an event rather than removing it.
There was a problem hiding this comment.
What is the proposed mechanism to enable this tracepoint?
#define it in the file and rebuild. Do you want me to document that too?
The corresponding entry tracepoint should be also be conditional so entry/exit tracing is balanced (and nesting is properly displayed when using -Xtrace:iprint...).
I'm aware (because this exit tracepoint was originally missing and so I added it to provide the balance you mention). But that causes a start-up regression. So I was trying to restore the "original" start-up performance without losing the knowledge that start-up performance is sensitive to this tracepoint's presence (even if disabled). I assume there is some diagnostic benefit to having at least the entry tracepoint, though I would guess it's not turned on very frequently and possibly even less frequently along with iprint.
What do you think of proceeding with this change as-is (to restore the world as it was before my effort to balance the tracepoints) and then we can open a follow-on issue to discuss how these verify tracepoints are used and decide whether the entry should also be taken out of the build by default?
There was a problem hiding this comment.
I would prefer to take Peter's suggestion to replace the tracepoint used at the start of the function with an "event" tracepoint. I think changing the existing tracepoint from TraceEntry to TraceEvent would qualify as a "signature" change (see doc/diagnostics/AddingTracepoints.md), so a first step could be to obsolete Trc_VM_performVerification_Entry and Trc_VM_performVerification_Exit then a new TraceEvent tracepoint to replace the former. Later we could decide whether we want to have a tracepoint at the return of the function.
Signed-off-by: Mark Stoodley <[email protected]>
|
Ok, i guess the other question is whether 1-2% startup regression in -Xint is worth mucking about with this RAS feature. I'll pursue that in the linked issue, since it's an internal one, and will summarize that discussion here. Honestly, I'm not sure it matters enough to go through the process Keith outlined. |
|
I don't see the suggestion here as onerous. What's the concern? |
Adds a J9VM_OPT_INCLUDE_CLASSVERIFY_EXIT_TRACEPOINT macro (not defined by default) to guard the recently (re?)introduced VerifyExit tracepoint, which resulted in some start-up regressions in -Xint mode.