pkg / errors

Simple error handling primitives
https://godoc.org/github.com/pkg/errors
BSD 2-Clause "Simplified" License
8.18k stars 691 forks source link

reduce allocations when printing stack traces #198

Open cstockton opened 5 years ago

cstockton commented 5 years ago

As requested in https://github.com/pkg/errors/pull/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.


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%
hanzei commented 5 years ago

@davecheney Could you please review this PR?

cstockton commented 4 years ago

Was this rejected for some reason or should I update it?

puellanivis commented 3 years ago

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?

cstockton commented 3 years ago

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.

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.

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