Add TraceFrom(pcs) for "on-demand" StackTrace instantiation #25
Conversation
Allows to record just the pcs from runtime.Callers when an error occurs and convert them to a StackTrace only if the trace is actually needed, e.g. when printing to a log. Improves performance about 4x.
|
This is an interesting idea. I would like to think about this a bit before deciding to merge it. In the mean time, would you kindly target this PR to the |
|
Re-targeted to |
|
Thanks for retargeting. I've taken a closer look at this PR and run the benchmarks. I'm persuaded that this would be a valuable addition, but I see some things we should improve before merging. I'll try post full review comments before the end of the week. |
| @@ -208,14 +208,24 @@ func (cs CallStack) Format(s fmt.State, verb rune) { | |||
| s.Write(closeBracketBytes) | |||
| } | |||
|
|
|||
| // Convenience wrapper around runtime.Callers() | |||
| func Callers(skip int) []uintptr { | |||
There was a problem hiding this comment.
I think we should choose a function name that aligns more closely to the Trace function since they both serve the same purpose just at different levels of abstraction. We should also improve the doc comment while we're at it.
Consider:
// TraceFast returns a low level representation of a CallStack. Use TraceFrom
// to convert the result into a CallStack. Use TraceFast as a lighter alternative
// to Trace when you want to record stack trace but will not always format it.
func TraceFast() []uintptr { ... }
| cs := make(CallStack, 0, n) | ||
| // TraceFrom creates a CallStack from the given program counters (as generated | ||
| // by runtime.Callers) | ||
| func TraceFrom(pcs []uintptr) CallStack { |
There was a problem hiding this comment.
The docs for TraceFrom should make it clear that it should only be given a return value from TraceFast and not just any slice of pcs. That gives us some wiggle room to adjust the implemenation without having to be compatible with arbitrary pcs []uintptr values.
| } | ||
|
|
||
| func BenchmarkCallers50(b *testing.B) { | ||
| b.StopTimer() |
There was a problem hiding this comment.
We should call b.StopTimer inside the loop before calling deepCallers like it is in BenchmarkCallers10. I realize you followed the pattern I had in the BenchmarkTrace functions but I did it wrong there and never noticed until now. Please fix those too if you don't mind.
|
@lukseven We may be able to get the performance gains you want without adding new functions to the API. This package used to be more efficient prior to the introduction of It would be great if we can get the lower upfront cost you want in the existing Let me know if you want to make an attempt at that. |
Allows to record just the pcs from runtime.Callers when an error
occurs and convert them to a StackTrace only if the trace is actually
needed, e.g. when printing to a log.
Improves performance about 4x.