-
Notifications
You must be signed in to change notification settings - Fork 20.6k
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
log: avoid stack lookups when not needed/used #28069
Conversation
}) | ||
} | ||
if locationEnabled.Load() { | ||
record.Call = stack.Caller(skip) |
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.
It's problematic. e.g. in the log/handler_glog.go
, L188, we assume this is available.
But I agree it's a good optimization, but just need more checks.
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.
Hm, interesting. I dug into this, it turns out that r.Call.String()
== %!v(NOFUNC)
.
due to
// String implements fmt.Stinger. It is equivalent to fmt.Sprintf("%v", c).
func (c Call) String() string {
return fmt.Sprint(c)
}
So there's no crash, just it won't print the backtrace.
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.
Should be noted, though: the Call
is stack.Call
-- not a pointer. Nothing is nil
here. And it goes on
type Call struct {
frame runtime.Frame
}
(no pointers)
type Frame struct {
// PC is the program counter for the location in this frame.
// For a frame that calls another frame, this will be the
// program counter of a call instruction. Because of inlining,
// multiple frames may have the same PC value, but different
// symbolic information.
PC uintptr
...
This struct too is (nearly) pointer-free.
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.
Ha I spent two hours yesterday trying to create a test to hit this, but couldn't because no pointer. So it looks good to me
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.
LGTM
Sweet. On my machine it's 1000ns vs 98 with and without the call. |
// storeStackRecords is an atomic flag controlling whether the log handler needs | ||
// to store the callsite stack. This is needed in case any handler wants to | ||
// print locations (locationEnabled), use vmodule, or print full stacks (BacktraceAt). | ||
var stackEnabled atomic.Bool |
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.
linter's gonna lint :P field and comment name don't match
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.
SGTM
Avoids the somewhat expensive stack.Caller invocation by checking if it is needed
Avoids the somewhat expensive stack.Caller invocation by checking if it is needed
This reverts commit f32ea1a.
This reverts commit f32ea1a.
We can avoid the somewhat expensive
stack.Caller
invocation by checking the atomic global.Closes #28064