search

go-stack / stack

Package stack implements utilities to capture, manipulate, and format call stacks.
MIT License
395 stars 33 forks source link

Add TraceFrom(pcs) for "on-demand" StackTrace instantiation #25

Open lukseven opened 3 years ago

lukseven commented 3 years ago

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.

coveralls commented 3 years ago

Coverage Status

Coverage increased (+0.2%) to 92.237% when pulling 301ad1487dfd72a401769cd88abcb8bbf994c2e0 on eluv-io:add-trace-from into 2fee6af1a9795aafbe0253a0cfbdf668e1fb8a9a on go-stack:master.

ChrisHines commented 3 years ago

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 develop branch instead? This project is using a git-flow branching model, so that master only gets new commits when releases are tagged. Thanks.

lukseven commented 3 years ago

Re-targeted to develop.

ChrisHines commented 3 years ago

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.

ChrisHines commented 3 years ago

@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 runtime.CallersFrames and adoption of that API. See the v1.7.0 release notes for some links with more details. But I have been reviewing the commit history for the implementation of runtime.CallersFrames and discovered that it was dramatically refactored in Go 1.12 in a way that I think makes it possible for this package to return to a lazy evaluation model without the issues introduced by Go 1.9 that we ran into before.

It would be great if we can get the lower upfront cost you want in the existing stack.Trace function and avoid adding two new functions that would clutter and confuse the package API.

Let me know if you want to make an attempt at that.