Optimise Msrv for common one item case

[GnomedDev]

Currently, Msrv is cloned around a lot in order to handle the #[clippy::msrv] attribute. This attribute, however, means RustcVersion will be heap allocated if there is only one source of an msrv (eg: rust-version in Cargo.toml).

This PR optimizes for said case, while keeping the external interface the same by swapping the internal representation to SmallVec<[RustcVersion; 2]>.

changelog: none

[rustbot]

r? @Centri3

rustbot has assigned @Centri3.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[flip1995]

r? @Jarcho as they looked into this most recently.

[Jarcho]

I don't see any reason to use ThinVec here over SmallVec which would be the same size and be simpler.

[GnomedDev]

SmallVec would be 24 bytes inline, as it stores the capacity and length inline in the heap allocated variant. I also didn't think it was already in the dependency tree, which this solution keeps the same. If you want to undo the ThinVec win, I can swap to SmallVec, but there is a benefit to this method.

[Jarcho]

Sorry for the long wait.

When I was doing the size in my head I was thinking RustcVersion used u32 instead of u16. It is indeed smaller. ThinVec has a small problem of requiring indirection for the case where there's no msrv and when the msrv is added via a crate level attribute rather than through one of the toml files. Given that we store less than fifty of these it's not really worth the extra code to use ThinVec over SmallVec

For memory savings Msrv is always stored in dynamically allocated memory, so the actual savings are less straightforward to determine. Anytime it's the only thing stored in the lint pass it probably does save some, but it very well might do nothing for the other cases.

[GnomedDev]

I cannot understand what you are saying here, and feel like one of us is confused?

This PR swaps Msrv to a custom enum which gets the best of both worlds, using an enum to hold 1 RustcVersion inline (ala SmallVec) and storing the uncommon case of using #[clippy::version] using ThinVec to shrink the inline size of Msrv as well.

If you dislike the complexity this adds, I can do two things:

• I can swap this PR to use SmallVec<[RustcVersion; 1]> to undo the inline size optimisation.
• I can swap this PR to just use ThinVec<RustcVersion>, to undo the "allocating for common case" optimisation.

Or, we can get this merged in as-is with both optimisations.

[Jarcho]

My main point is that ThinVec may not actually be an optimization. There are a few cases to handle, and only one of them is definitely helped by ThinVec being used.

• An msrv is set only in either toml file. This case gets a definite improvement.
• An msrv is not set anywhere. This case takes up less room since the empty vec is shared, but it loads the second cache line when checking for items.
• An msrv is set in a crate level attribute. SmallVec wins this one unless you specifically handle switching from zero to one.
• The rare cases where multiple msrvs are set. I'm willing to not care about this, but SmallVec does better here.

My second point was about the actual memory savings. Since Msrv is always stored in boxed structures there are cases where it won't save any memory at all. Memory allocators don't allocate exact amounts of memory, especially for small objects. Reducing the size from 24 to 16 probably does allocate less, but plenty of the lint passes also hold other data as well. This means determining the amount of memory actually saved is non-trivial.

---

Basically don't use ThinVec unless it's well justified. Since only a small number of Msrv structures are created (about 50), it doesn't stand to save very much memory. It has a common case where it may perform worse and it has a rare case where it definitely uses both more memory and runs slower.

[GnomedDev]

but it loads the second cache line when checking for items.

What are you talking about here?

An msrv is set in a crate level attribute. SmallVec wins this one unless you specifically handle switching from zero to one.

I am explicitly handling this case https://github.com/GnomedDev/rust-clippy/blob/a83cf350e50ef2b438a02aa17e3980f012e59b65/clippy_config/src/msrvs.rs#L170

---

Anyway, I'll change it to SmallVec<[RustcVersion; 1]> now, since this isn't getting merged in otherwise.

[GnomedDev]

After a tiny bit of testing, turns out we can fit 2 RustcVersions inline without increasing the inline size, so I did that instead.

[Jarcho]

but it loads the second cache line when checking for items.

What are you talking about here?

The call to last always reads the size in ThinVec's header. This means accessing the last element has to load the cache line containing the header which is a different than the cache line containing the pointer. Going through ThinVec's API there is a has_capacity function which would let you avoid that.

---

Anyways, thank you. @bors r+

[bors]

📌 Commit d0b15f1 has been approved by Jarcho

It is now in the queue for this repository.

[bors]

⌛ Testing commit d0b15f1 with merge a529c44...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing a529c44 to master...