GEN-203: `Report` could use `ThinVec<Frame>` instead of `Box<Vec<Frame>>`

[GnomedDev]

Related Problem

Box<Vec<Frame>> is double indirection, but is being used here to minimize the inline size so the box_collection clippy lint is being silenced.

Source:

hash/libs/error-stack/src/report.rs

Line 251 in 958822e

pub(super) frames: Box<Vec<Frame>>,

Proposed Solution

The ThinVec crate can be used to remove the double indirection while keeping the single inline pointer size by storing the capacity and length at the start of the allocation.

This crate is trusted by rustc, so shouldn't have any issues with trusting a new dependency.

Alternatives

Keep as-is which will continue the double indirection, although on the cold path.

Additional context

No response

[TimDiekmann]

Hi @GnomedDev and thanks for the suggestion and it definitely makes sense! I'd probably would make this an optional (enabled-by-default) feature.

I'll defer to @indietyp here as he has planned on do some changes in regards to the double indirection in the future anyway.
Related PR: #5161

[indietyp]

Thank you!

I really like the idea, I've already experimented in the branch mentioned by Tim to reduce the amount of allocations and indirection by having a set of Vec where we store the frames in.

We've put it on draft for now, just because it is quite a bit more unsafe code, but might pursue it in the future. In the meantime I think moving to ThinVec could be beneficial. The only problem I see is that right now we're using a Box<[Frame]> to store the sources of a frame, that would no longer really work, because according to ThinVec (see: https://docs.rs/thin-vec/latest/thin_vec/struct.ThinVec.html#method.from-7) this will re-allocate. I am unsure if it's worth the trade-off or if we want to just use ThinVec everywhere. In that case I am unsure, because I like the idea of having a boxed slice, it allows us to ensure that the amount of sources does not change.

I would either want this to be always included or a feature that is not enabled by default - the reason is simple - as a library author I might not say default-features = false in that case ThinVec would always be included anyway and it just needs a single crate for that to be true. In that case it might just be easier to include it by default or have it as a feature that the application may choose to enable.

I also doubt that in the cold path the double indirection really matters. Because every change_context or attach is always a new allocation it would be faster to simply allocate in an arena and reduce the amount of allocations needed, especially as this isn't the hot path this would be more likely than not a greater efficiency bonus than eliminating the single double indirection.