reduce allocations when printing stack traces #198
Conversation
Use a bytes.Buffer grown to len(stack)*stackMinLen for writing to format. ---- benchmark old ns/op new ns/op delta BenchmarkStackFormatting/%+v-stack-10-24 20692 16377 -20.85% BenchmarkStackFormatting/%+v-stack-30-24 43502 30200 -30.58% BenchmarkStackFormatting/%+v-stack-60-24 43166 29790 -30.99% benchmark old allocs new allocs delta BenchmarkStackFormatting/%+v-stack-10-24 19 6 -68.42% BenchmarkStackFormatting/%+v-stack-30-24 33 3 -90.91% BenchmarkStackFormatting/%+v-stack-60-24 33 3 -90.91% benchmark old bytes new bytes delta BenchmarkStackFormatting/%+v-stack-10-24 1427 2942 +106.17% BenchmarkStackFormatting/%+v-stack-30-24 3341 6261 +87.40% BenchmarkStackFormatting/%+v-stack-60-24 3341 6261 +87.40%
|
@davecheney Could you please review this PR? |
|
Was this rejected for some reason or should I update it? |
|
Are programs really entering into these functions so often? Ideally, error pathways should be uncommon, especially those that attach a stack trace. I’m not sure if any of these saved allocations would turn into anything meaningful in the real world. Benchmarks demonstrate that there is performance improvement here, sure… but are we going to be calling these at anymore than maybe 10–100 qps? |
|
So the original change in #150 was much more impactful, at 90-97% memory & 50% CPU reductions. It was merged by hand which missed some changes as well as causing me to lose authorship for the work. I was asked to include this pull request to get the last little bit of performance.
In general you hope that your app doesn't fail often, but when it does having a failure path that deviates greatly in resource cost can be very unpredictable. Given the ubiquitous usage of the errors package I think these allocation reductions have a real world impact for failing Another reason why I included this change is because of the potential security implications. Any request path an attacker can find that causes a 90% increase in memory can be a potential attack vector for a DDoS. Given that errors tend to sit in the edge of your request paths in front of rate limiters and other front line defense, needless allocations make the attack cheaper. That all said I see no reason not to include this change as it doesn't have any cost in complexity or anything else, it's very straightforward: https://github.com/pkg/errors/pull/198/files |
As requested in #150 - I made the minimal changes here you requested.
Small note: it would be nice if the standard library https://github.com/golang/go/blob/master/src/fmt/print.go#L186 implemented
type Grower interface { Grow(n int) }so we could assert for it instead, but no idea if that would be considered or not.