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
Ensure to check the runtime log level for RequestContext tracing needs #27736
Conversation
@@ -126,7 +122,7 @@ public void destroy(Contextual<?> contextual) { | |||
|
|||
@Override | |||
public void activate(ContextState initialState) { | |||
if (traceEnabled) { | |||
if (LOG.isTraceEnabled()) { | |||
String stack = Arrays.stream(Thread.currentThread().getStackTrace()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated but useful: always move away "unlikely" paths ie cold ones out of the "main" method logic: it would increase chances to inline it (see http://normanmaurer.me/blog_in_progress/2013/11/07/Inline-all-the-Things/ that's still very actual and these things hasn't changed across JDK versions, really).
Most inlining decisions on OpenJDK happens based on the bytecode size (although not always; sometime the native method size is important too), meaning that using any bytecode plugin viewer (I use https://plugins.jetbrains.com/plugin/9248-jclasslib-bytecode-viewer) can help checking how much the bytecode size change while grouping differently code into methods:
eg
if (LOG.isTraceEnabled()) {
traceActivate(...);
}
this is going to reduce a bit the bytecode size of the original method, increasing its chance to be inlined in the common use case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very true, but in this case I preferred the simplest patch to expedite integration & minimize friction for backports.
Definitely something to consider if someone wants to get back to this and improve further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree!
Failing Jobs - Building 854e87a
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 18 #- Failing: extensions/smallrye-reactive-messaging-amqp/deployment
! Skipped: integration-tests/reactive-messaging-amqp 📦 extensions/smallrye-reactive-messaging-amqp/deployment✖
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've verified in our perf lab that's going to fix the performance regression seen on current master.
It goes without saying that this should be most important thing we backport to |
Fixes #27735
Fixes #27743