stack icon indicating copy to clipboard operation
stack copied to clipboard

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

Open lukseven opened this issue 4 years ago • 5 comments

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.

lukseven avatar Jan 27 '21 13:01 lukseven

Coverage Status

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

coveralls avatar Jan 28 '21 02:01 coveralls

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.

ChrisHines avatar Jan 30 '21 20:01 ChrisHines

Re-targeted to develop.

lukseven avatar Feb 01 '21 09:02 lukseven

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 avatar Feb 04 '21 05:02 ChrisHines

@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.

ChrisHines avatar Feb 08 '21 03:02 ChrisHines