RFC: Switch from tinyvec to smallvec

[emilio]

At least in Firefox, we use SmallVec extensively, and I'd love to avoid having two data structures doing fundamentally the same thing. It seems SmallVec is more popular in the ecosystem than TinyVec, so switch to it.

r? @Manishearth

[madsmtm]

Just noting that a selling point of TinyVec is that it uses 100% safe code - but smallvec seems extensively verified, so probably not that important

[Manishearth]

Yeah, it was switched over for that purpose (#54)

I personally do not feel strongly about the switch and am okay switching it back though.

smallvec has had a couple unsafe vulns and perhaps should have some of the unsafe code cleaned up but it is a bit of an ecosystem standard and this crate is found transitively in a lot of stuff. Furthermore we make rather simple use of it and aiui none of the vulns have actually affected us.

In general I don't put much stock in switching deps just because of the presence of unsafe code.

@emilio still, would you consider trying to get rid of the unsafe in smallvec? It's definitely possible now

[madsmtm]

Also tagging the author of #54: @Shnatsel

[emilio]

@Manishearth I'm happy to give that a shot. Though that said there are unsafe things in SmallVec which TinyVec just doesn't do, like implementing Send and Sync. I'm assuming you don't mean those though.

[Manishearth]

I highly suspect those will be unnecessary after it's switched to pure safe code 😄

Send and Sync impls are only necessary when doing unsafe stuff, otherwise they're autoimpld

[Manishearth]

A downside of the tinyvec approach is that it forces initialization of each field, unsure if that can be done in smallvec.

I think even just limiting the unsafe to MaybeUninit on the array would help. There's no need for smallvec to carry its own resizeable buffer impl.

[emilio]

@Manishearth I'm also not quite sure how you'd go about removing unsafe fully, actually. It seems you need MaybeUninit for the inline storage at the very least. TinyVec enforces T: Default to avoid this / always initializes the inline buffer. Which is fine for really tiny buffers like this but not for other stuff we do in Gecko.

[emilio]

Hah, mid-aired with you :)

[emilio]

Anyways, this discussion is somewhat tangential to the PR. Another thing we could do is just adding a feature that allowed you to choose which of tinyvec / smallvec you want in this crate, but that all seems a bit overkill.

[Shnatsel]

Indeed, the ideal endgame is removing most of the unsafe code from SmallVec. A lot of it doesn't really have to be there. The inline storage does need MaybeUninit and the associated unsafe, but we could simply reuse the arrayvec crate that already implements all that. In fact, simply taking the TinyVec codebase and swapping their inline storage tinyvec::arrayvec for the arrayvec crate should get us 90% of the way to a much safer SmallVec implementation. I wanted to do that and propose it as SmallVec 2.0 for ages, but never got the time to actually do it.

For now I would prefer to stick to TinyVec for unicode-normalization, simply because it doesn't need the inline MaybeUninit that SmallVec provides and so doesn't need to pull in the unsafe code.

May I ask what is your concern with pulling in TinyVec in Firefox? It is 100% safe code, has been verified for correctness against std::Vec as a reference via fuzzing, and has a very small supply chain footprint. This is a dependency with as few downsides as can be.

[emilio]

Mostly not having to pull and audit unnecessary dependencies.

[Shnatsel]

An explicit design goal of tinyvec was the ease of auditing, with the code being 100% safe, no mandatory dependencies, and no fancy tricks like build.rs. What makes tinyvec problematic to pull or audit?

[emilio]

Nothing, other than "unicode-normalization is the only place we'd use it, and we extensively use another crate for the exact same purpose". It's not hard or problematic, it's just unnecessary if we have another crate doing the same thing.

[Shnatsel]

There are a lot of codebases out there that do use unicode-normalization but not SmallVec, and auditing SmallVec is no small feat - there's a lot of fragile unsafe code. That's a big part of the reason why TinyVec exists (along with the past SmallVec vulns).

So I don't think it makes sense to upstream this change, but you can carry it as a local patch if you really want to avoid an extra dependency, even if it's an innocuous one.

[Manishearth]

Yeah, but in general smallvec is much more used in the ecosystem and I think if we're talking about footprint sticking to the ecosystem standard also makes sense, especially when we're making use of the crate

I'm fine with a feature, but I would like to switch back to smallvec if it reduces the amount of unsafe code to just a maybeuninit array. I think arrayvec might also work but it's not clear if that can be easily internally swapped without a breaking change.