make it easy to resolve symbols by frame

[fraillt]

Hello,

Resolving backtrace (at least for the first time) is very slow, but this is not something that can be solved (I guess). However, not all frames are equally useful, and at least in the code base that I'm working on, we simply ignore bunch of them. We have code something like this:

let mut bt = Backtrace::new_unresolved();
// ...
bt.resolve();
let bt: Vec<String> = bt.frames()
    .iter()
    .flat_map(BacktraceFrame::symbols)
    .take_while(is_not_async_stuff)
    .map(format_symbols)
    .collect();

For us, it takes takes around ~700ms (sometimes even ~1.5s) to resolve whole backtrace.

With this PR we're able to write like this:

let bt: Vec<String> = bt.frames_mut()
    .iter_mut()
    .flat_map(BacktraceFrame::symbols_resolved)
    .take_while(is_not_async_stuff)
    .map(format_symbols)
    .collect();

And it takes around 25ms to resolve the frames that we actually need.

I'm aware of low level utils backtrace::trace and backtrace::resolve_frame, but this would basically mean that I would basically need to reimplement Backtrace, including Serialize/Deserialize capability, which is too much.

[workingjubilee]

I think the motivation you have is good. But I feel slightly nervous about exposing &'a mut [BacktraceFrame] and thus &'a mut BacktraceFrame, allowing mutations that can make the backtrace inconsistent. This is partly because I've noticed the problems you observe, and I think your direction overall is a good idea: making this crate's backtrace lazier is clearly the "way to go".

But exposing the entire slice mutably, and without resolving first, is potentially problematic for later optimizations and better interfaces that might rely on internally avoiding doing work where unnecessary, but now would have to account for "you might have called frames_mut and done something with them first". So I think addressing the needs you've cited might be better done by adding a function to Backtrace that returns an iterator that lazily resolves a BacktraceFrame on iteration.

[fraillt]

@workingjubilee Thanks for your insights, it makes sense. I tried to implement lazy iterator, but it's not straightforward as it requires interior mutability. I noticed that we already have From and Into<Vec<BacktraceFrame>> for Backtrace so I guess we can simply provide a way to resolve each frame manually. (I updated revision with minimum changes)

[github-actions[bot]]

Below is the size of a hello-world Rust program linked with libstd with backtrace.

Original binary size: 518984B Updated binary size: 519008B Difference: +24B (0.00%)

[fraillt]

Summary of all changes so far:

• provided new public function BacktraceFrame::resolve(&mut self) (this is the essence of the PR)
• replaced impl Into<Vec<BacktraceFrame>> for Backtrace with impl From<Backtrace> for Vec<BacktraceFrame>

  One should avoid implementing Into and implement From instead. (source)

• don't expose/store backtrace implementation frames. Now Backtrace::frames(&self) returns same frames as in impl From<Backtrace> for Vec<BacktraceFrame>.

[workingjubilee]

replaced impl Into<Vec<BacktraceFrame>> for Backtrace with impl From<Backtrace> for Vec<BacktraceFrame>

I know that's the "correct" style, but we've had several breakages due to "fixing" this, because this crate gets included directly into std, and sometimes rustc gets confused as to whether impls in this crate are publicly accessible, especially when it comes to resolving type inference.

So, unfortunately, I must tell you: Do it wrong.

[fraillt]

Thanks for fast replay! but I really thing that impl From<Backtrace> for Vec<BacktraceFrame> is not an issue. There's my reasoning. There's conversion only from Backtrace to BacktraceFrame:

• both are local types, and not widely used.
• both are not generic "container like types", so there's no transitive converions (e.g. impl From<Vec> for Backtrace) or something) Type inference here should be pretty easy here, and there are a lot of code like this in the the wild. I really cannot see how this could interfere with other type coercions... :(

If this is really a problem, then: a) I think the problem is elsewhere, and we should fix that problem instead :) b) we should be able to "proof" that there's a problem, revert it back and add big comment in code, explaining exactly that is the issue.

[workingjubilee]

Type inference here should be pretty easy here, and there are a lot of code like this in the the wild. I really cannot see how this could interfere with other type coercions... :(

Part of why it is not disruptive "in the wild" is because there either already is two implementations to infer types from, or all the types to infer based on local implementations are local and it resolves one of those. There is an unfortunate quirk where if there is only one implementation it is always inferred by default, even though it would be ambiguous if there were two in scope. Thus adding additional directions for Vec<T> can weaken type inference even though, by all rights, if the type inference algorithm did not have this quirk, it would not infer in the first place (and possibly instead use some other information to guide inference).

std is unique in appearing in what rounds off to every single crate. Specifically, Vec<T> is in scope in every single crate.

[workingjubilee]

So I agree the problem is elsewhere, but I did not want to block accepting this PR on landing a new type inference algorithm in rustc, or making sure type resolution + inference never can "leak" in a more "by construction" manner than how it is currently done.

[github-actions[bot]]

Code size changes for a hello-world Rust program linked with libstd with backtrace:

On platform windows-latest:

• Original binary size: 140,800 B
• Updated binary size: 138,752 B
• Difference: -2,048 B (-1.45%)