Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reword or even allow-by-default box_vec #2404

Open
killercup opened this issue Jan 27, 2018 · 3 comments
Open

Reword or even allow-by-default box_vec #2404

killercup opened this issue Jan 27, 2018 · 3 comments
Labels
A-documentation Area: Adding or improving documentation

Comments

@killercup
Copy link
Member

killercup commented Jan 27, 2018

In #2394 (comment) (a typical "only 20%-on-topic comment" of mine), I wrote

But thinking about the box_vec lint I'm not sure I agree with its reasoning:

Vec already keeps its contents in a separate area on the heap. So if you Box it, you just add another level of indirection without any benefit whatsoever.

There is a benefit I can think of: Boxing a Vec or String reduces the size from 3 words to 1. I'm sure some this may be a good idea, and, more importantly, this is most likely done on purpose. But maybe this is just a good idea in my imagination. Or maybe there is a crate that gives you a Vec-like API but stores length an capacity as part of the heap data and we should suggest using that?

And indeed—there is a crate that does just that, and it is @gankro's thin-vec. I'll allow the fact that it is not published to crates.io and has no readme to count for "this is an obscure use case".

Update: But it may be used in Gecko in the future (source).

So, I'm totally fine if nobody cares about this (I'll most likely never use this myself), but can I suggest changing the box_vec description to this? :)

Vec already keeps its contents in a separate area on the heap. So if you Box it, you add another level of indirection. This is often not the programmer's intention.

In the case you want to reduce the size of item on the stack, you should consider using a special Vec-like data structure that stores its length and capacity on the heap.

@oli-obk oli-obk added the A-documentation Area: Adding or improving documentation label Jan 27, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Jan 27, 2018

I'd go with the docs case, and if there's ever a crate on crates.io, we can add a suggestion to use it to the lint message

@Gankra
Copy link

Gankra commented Jan 29, 2018

Since I'm cited in this I should note that ThinVec is only reasonable by virtue of the boxing being "builtin" -- Box<Vec> is indeed a suspicious thing outside of very degenerate situations.

@MoSal
Copy link

MoSal commented Oct 22, 2019

So I have a use-case where large (but not huge) data is deserialized, and the deserialized data must be kept in memory as long as the app/service is running.

struct ChildA {
 id: String,
 foo: Option<Vec<Foo>>,
 bar: Option<Vec<Bar>>,
 // 10's of other elements that are either
 // Option<Vec<T>> or Option<T>, most of which
 // are None most of the time
}

struct ChildB {
 // similar to ChildA
}

struct Root {
 // some general info elements
 // ----------
 // // the number of elements in these vectors is big
 a: Option<Vec<ChildA>>,
 b: Option<Vec<ChildB>>,
}

Boxifying vectors (and other large types) in ChildA/ChildB makes a huge difference.


I think a useful heuristic would be to special-case if the boxed vec belongs to an enum variant, and allow the boxification if it reduces the enum size, in other words, letting large_enum_variant take over. If you think the boxed vector is going to be empty most of the time, then maybe you should put it inside an Option anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documentation Area: Adding or improving documentation
Projects
None yet
Development

No branches or pull requests

4 participants