Skip to content
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

Stop querying for stack frames multiple times on CallerInfo() #7

Merged
merged 4 commits into from
Jun 24, 2024

Conversation

mikeauclair
Copy link

@mikeauclair mikeauclair commented Jun 24, 2024

Summary

  • Switches CallerInfo to use runtime.Callers() to pull the stack instead of repeatedly calling runtime.Caller()

Changes

  • Switch to a single call of Callers() instead of repeated Caller invocations
  • Create a slice with cap 50 to store the frame pointers (Zero percent married to this value)
  • Iterate over the frames with the frames.Next() iterator

Motivation

  • We heavily use mocks at DevotedHealth and the repeated calls to pull the stack were flagging as a contributor when I profiled some of our slow tests - see example of Caller contributing a CPU cost of 7660ms out of a total cost of 13740ms
Screenshot 2024-06-24 at 1 10 26 PM

Related issues

Copy link

@dchang-dchang dchang-dchang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with 50. It would be nice if n==50 that we added a "more callers ignored" or something as the last "caller" just as a warning to users.

@mikeauclair
Copy link
Author

I'm ok with 50. It would be nice if n==50 that we added a "more callers ignored" or something as the last "caller" just as a warning to users.

That totally makes sense - let me see if I can figure out how to actually test that before I commit to adding it, though

@dchang-dchang
Copy link

I didn't realize what repo I was in - this is also fine to panic as this will be all tests

@dchang-dchang
Copy link

and if you're going to upstream this change, presumably, they'd like a version where you loop and get all of them as the previous version does - just 10-50 at a time

@mikeauclair
Copy link
Author

and if you're going to upstream this change, presumably, they'd like a version where you loop and get all of them as the previous version does - just 10-50 at a time

Yeah I think I'm actually just going to do that now that I think about it

dchang-dchang
dchang-dchang previously approved these changes Jun 24, 2024
Copy link

@dchang-dchang dchang-dchang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! A small nit - I think it's marginally easier to read and marginally less instructions but I'm good either way.

(would be nice to have a test to prove that it works for bigger stack depths - not sure if that's easy to do or hard?)

pcs := make([]uintptr, stackFrameBufferSize)
offset := 1
n := runtime.Callers(offset, pcs)
maybeMore := true

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybeMore := n == stackFrameBufferSize and you can tuck it behind the n==0 early return a couple lines down

}
offset += stackFrameBufferSize
n = runtime.Callers(offset, pcs)
if n < stackFrameBufferSize {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same nit here

@mikeauclair
Copy link
Author

(would be nice to have a test to prove that it works for bigger stack depths - not sure if that's easy to do or hard?)

It's hard because they explicitly opt stack frames from the testify packages out of accumulation 🙃

@mikeauclair mikeauclair merged commit 7ecde95 into master Jun 24, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants