Add a push_light method and use likely/unlikely intrinsics #134
Conversation
| } | ||
|
|
||
| #[cfg(not(feature = "push_light"))] | ||
| fn push_light(&mut self, _: T) {} |
There was a problem hiding this comment.
I don't understand why this is defined if the feature is missing, and why it does nothing.
There was a problem hiding this comment.
It's to ensure things still compile
There was a problem hiding this comment.
It compiles sure, but it doesn't do the right thing, why should it be a noop if the feature is missing? Sounds like a way to have silent bugs.
There was a problem hiding this comment.
It's not meaningful to benchmark a feature if it's disabled
There was a problem hiding this comment.
Oh ok sorry I didn't notice this was an implementation for benchmarks.
|
|
||
| trait Vector<T>: for<'a> From<&'a [T]> + Extend<T> + ExtendFromSlice<T> { | ||
| fn new() -> Self; | ||
| fn push(&mut self, val: T); | ||
| fn push_light(&mut self, val: T); |
There was a problem hiding this comment.
IMO this should either not exist when the feature is not enabled, or delegate to push in the default implementation.
|
So the speedup when the vector isn't spilled is more than 2x (both with and without inlining, I added benchmarks to test it without the |
|
It probably does make sense to make it the default behavior. |
|
Additionally, I would mark this function |
|
☔ The latest upstream changes (presumably #169) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Was this ever made the default behaviour? It seems like an easy win. |
|
We can't make this the default as written, because even without the |
|
I pushed a rebased version of this PR plus a commit to make At a glance, it looks like this is still a win for |
|
You can always have a |
|
Yes, the branch I linked above keeps the unstable intrinsics behind an optional feature. |
This adds a
push_lightalternative topushwhich is optimized for inlining and for vectors which fit inside the fixed size.This change is