Skip to content

Implement try_with_capacity and improve with_capacity - #574

Open
bolshoytoster wants to merge 1 commit into
servo:v2from
bolshoytoster:v2
Open

Implement try_with_capacity and improve with_capacity#574
bolshoytoster wants to merge 1 commit into
servo:v2from
bolshoytoster:v2

Conversation

@bolshoytoster

Copy link
Copy Markdown
Contributor

Closes #416.

Also makes with_capacity ~12% faster in the heap case.

@alejandro-vaz

Copy link
Copy Markdown
Collaborator

I think the issue underlying the codebase is much bigger than "we had an unoptimized implementation"

if you think about it, why is that faster than what we had?? well, it directly gives information to the compiler of some invariants and changes code paths knowing that

before, with_capacity didn't know that the instance could never be spilled and it's length must equal zero because we just created it, so we had additional checks and stuff

I don't think this is an issue of "we didn't optimize this function", it is "we have a shitload of methods and implementations on a 2.5k LOC file and surprisingly many of them aren't even doing what they are supposed to do in terms of the abstraction"

this may be a bit extreme but I'm proposing a lib.rs rewrite from scratch. lib.rs is currently blocking:

and we can't even trust the current implementations in it to be neither optimal nor fully safe (remember that memory leak that appeared from nowhere in #450 or the stacked borrow violation in #449)

I haven't been opening PRs recently because I have like 5 branches discarded and 20 half-implementations that came across restructuring errors (e.g. manual iterator field magic, spec_traits on random places, etc). this has also happened to some of you

more than a 2k LOC replacement PR it would look more like: delete most lib.rs, comment everything that is giving errors elsewhere, and then reimplement everything one step at a time to bring it all online

if modularization has shown something it's that simple modules were easily extracted, core stuff was all intertwined

this optimization originated in #500 (comment)

we actually shouldn't be optimizing in an alpha version. we are still in alpha because we are missing features, https://github.com/servo/rust-smallvec/wiki/on-v2%E2%80%90alpha-performance-contributions

Request For Comments @bolshoytoster @TDecking @fereidani

@fereidani

fereidani commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

I have a problem with not optimizing in alpha stage, I experienced that it is hard to optimize a project when it is nearly finished and already have made wrong choices (explained my opinion in https://fereidani.com/optimization-first-rule-dont-but-aim-for-it).
If we delay optimization, when we will have time to review these unsafe codes? during beta? what if optimization requires structural changes or other rewrites?
But I believe we need to have a standard about what optimization code should get merged, for example threshold of how many lines it adds and what affects it has on the binary size of the project(how many instructions add or removed).
Following standard library path is the best guideline at least for now.
I was considering adding unreachable_unchecked to these reserve functions for further improvements too.
They have massive gains by giving compiler hints about the success/need of reallocation.
These capacity related optimizations have huge impact on performance of the smallvec and we should consider implementing them, both hot and cold variants. we can refactor them to another file to keep them contained if you are worried about lines of code.
I generally believe any optimization that 1.reduces instruction counts and 2.improves performance must get merged if and only if proved completely sound.

@alejandro-vaz

alejandro-vaz commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

I agree with @fereidani

that's the stance I'm also defending

though I'm not in favor of optimizing what we have. why doesn't reserve use unreachable_unchecked already for example?? why didn't the original implementation already come with it?? that should have been done

but instead of making things one I have the impression we are chasing our tail by repeatedly optimizing stuff instead of committing a really good implementation

why are we fixing technical debt that's deep into the repo??

honestly the "no performance contributions in alpha" was mostly an ad-hoc rule to focus on architecture first (length, modularization) and avoid trying to fixate ourselves on making small functions a bit faster. the endless structs and assert functions were also pretty annoying, not because they are bad by themselves, but because having them inline is noisy and a module would have been preferred instead, I am in favor of doing so

If we delay optimization, when we will have time to review these unsafe codes? during beta? what if optimization requires structural changes or other rewrites?

why not make it once and properly instead of having to review and refactor code that's four years old whilst having to remake core parts of the architecture??

why don't we center our architecture first around performance and not let anything in that's half-baked?? and we are suddenly realizing we are leaving massive gains everywhere??

what I'm most worried about is that when I have to implement something or review something, there's no "default obvious right way of doing so that is fast", but surprisingly 10 different ways with 4 different helper methods that all do pretty much the same except that three do not make assumptions that can be made and are slower

about the hot and cold variants, I've never gone deep into that stuff so it looks a bit weird for me, but I guess it makes sense. it's annoying to me but I can understand it, it will make sense once I get more used into that

every time you try to understand a new function, like retain_mut now, that implements really good optimizations, you need to discover on-the-fly what kinds of optimizations it uses and who knows whether other functions use the same ones. we have a ton of structs that do stuff on drop for optimizing code / better panic handling, why don't we have them somewhere that we can understand them and make sense of them as a whole

my suggestion is to make a rewrite with the optimizations (which often just means a good architecture and the micro-stuff) instead of having to audit the ~30 possible methods that are not optimized and that do right-but-slightly-unrelated stuff like the current implementation this PR is trying to change

@alejandro-vaz

Copy link
Copy Markdown
Collaborator

by the way, that post is great. I haven't read it fully but it's really good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement try_with_capacity

3 participants