Speed up push by expanding triple_mut

[bolshoytoster]

See #379.

This seems to increase binary size by 32 bytes (on a release build, it's closer to 896 in debug), although it might end up being more depending on how many times push is inlined.

[alejandro-vaz]

32 bytes can be safely ignored as not meaningful

[alejandro-vaz]

can you paste the improvements table??

[bolshoytoster]

Current non-union:
test bench_push                        ... bench:         413.05 ns/iter (+/- 11.63)
test bench_push_small                  ... bench:          52.97 ns/iter (+/- 3.21)
test bench_push_vec                    ... bench:         566.10 ns/iter (+/- 21.99)
test bench_push_vec_small              ... bench:          58.42 ns/iter (+/- 3.26)
test bench_pushpop                     ... bench:         711.49 ns/iter (+/- 26.09)
test bench_pushpop_vec                 ... bench:         292.66 ns/iter (+/- 7.47)

New non-union:
test bench_push                        ... bench:         390.38 ns/iter (+/- 18.83)
test bench_push_small                  ... bench:          39.83 ns/iter (+/- 6.27)
test bench_push_vec                    ... bench:         414.47 ns/iter (+/- 16.19)
test bench_push_vec_small              ... bench:          46.17 ns/iter (+/- 4.24)
test bench_pushpop                     ... bench:         581.87 ns/iter (+/- 104.08)
test bench_pushpop_vec                 ... bench:         233.12 ns/iter (+/- 27.54)

Current union:
test bench_push                        ... bench:         610.56 ns/iter (+/- 12.40)
test bench_push_small                  ... bench:         100.19 ns/iter (+/- 2.04)
test bench_push_vec                    ... bench:         623.76 ns/iter (+/- 25.83)
test bench_push_vec_small              ... bench:          52.79 ns/iter (+/- 5.81)
test bench_pushpop                     ... bench:       1,080.09 ns/iter (+/- 42.82)
test bench_pushpop_vec                 ... bench:         270.46 ns/iter (+/- 11.53)

New union:
test bench_push                        ... bench:         352.68 ns/iter (+/- 12.00)
test bench_push_small                  ... bench:          37.70 ns/iter (+/- 0.74)
test bench_push_vec                    ... bench:         395.48 ns/iter (+/- 27.31)
test bench_push_vec_small              ... bench:          43.04 ns/iter (+/- 2.02)
test bench_pushpop                     ... bench:         659.03 ns/iter (+/- 47.59)
test bench_pushpop_vec                 ... bench:         220.82 ns/iter (+/- 7.93)

[alejandro-vaz]

improvements are really striking for what the change does

[alejandro-vaz]

was this related to #361??

[bolshoytoster]

was this related to #361??

I still can't reproduce that, but this should improve that anyway.

[alejandro-vaz]

I'm not sure I can justify a full patch release from this even though improvements are meaningful

we should try to avoid releasing two versions within three days at least, as a rule of thumb

this undoubtedly should be published soon

[bolshoytoster]

I've just noticed that insert has the same pattern:

rust-smallvec/src/lib.rs

Lines 1461 to 1467 in fadb3b8

	let (mut ptr, mut len_ptr, cap) = self.triple_mut();
	if *len_ptr == cap {
	self.reserve_one_unchecked();
	let (heap_ptr, heap_len_ptr) = self.data.heap_mut();
	ptr = heap_ptr;
	len_ptr = heap_len_ptr;
	}

That might also be improvable. I'll look into it.

[alejandro-vaz]

is it a case against using triple and triple_mut altogether or is it just specific to those operations??

if triple and triple_mut are consistently slower everywhere, we may as well remove them

[bolshoytoster]

The problem with this optimization is the potential bloat (and it makes the code more verbose/less maintainable), so it's probably only worth adding it to hot code.

[alejandro-vaz]

code can always be refactored, I don't think that's an issue

I haven't started going intra-module to see how to refactor things much, but there is a lot of unnecessary stuff everywhere

if you look at it, it's the same pattern always

you can potentially extract that to a function that's always inlined which takes a impl FnOnce(/* args of triple */) -> whatever which is the body of it and the compiler should optimize it the same way

it just looks so surreal that moving that affects codegen so much

[bolshoytoster]

I've implemented this for insert, but the benchmark results aren't much different. Although, I accidentally found that it does speed up inserting to the end of a SmallVec (just a push), so I assume the reason results are the same is because most of the cost is in copying elements, making this optimization insignificant.

Should I submit it anyway, even if the change is unnoticable in most cases, and it makes the code harder to read?

Unrelated: Since reserve_one_unchecked is only used in push and insert, always in this pattern, would it be better to have it return heap_mut, i.e.

fn reserve_one_unchecked(&mut self) -> (NonNull<A::Item>, &mut usize) {
    debug_assert_eq!(self.len(), self.capacity());
    let new_cap = self
        .len()
        .checked_add(1)
        .and_then(usize::checked_next_power_of_two)
        .expect("capacity overflow");
    infallible(self.try_grow(new_cap));
    unsafe { self.data.heap_mut() }
}

Then the if blocks that are repeated could just be:

if *len_ptr == self.capacity {
    (ptr, len_ptr) = self.reserve_one_unchecked();
}

I've checked, and this lowers binary size in both debug and release builds.

[alejandro-vaz]

I don't think it's a good idea to complicate insert if there are no performance gains. it's true that your way is more logically correct and in normal conditions I'd certainly prefer it, but this codebase is already bloated everywhere, right now it's not a good idea

if the private function reserve_one_unchecked is only used twice and we are not sure we want its exact behavior, why the hell is it a function then?? just inline copy-paste the code in those two places and adapt it needed. what a clown function

[bolshoytoster]

why the hell is it a function then??

It's worth noting that it's marked #[cold] to stop it from being inlined, and it's part of an optimization + using it is more concise than copy-pasting the body everywhere.

[alejandro-vaz]

okay, can we do what it does in a simpler way??

it makes sense that removing that function would also put in doubt reserve and all others so it's probably not a good idea, you are right

we currently have like four different reserve functions that I've seen and #562 wants to add more on v2

since this is v1, I think it's better to shrug our shoulders

if there are performance gains by making it return heap_mut, then go ahead

[bolshoytoster]

I've benchmarked a few different combinations of changes, these are the ones that weren't eliminated:

	Debug	Release
reserve_one_unchecked -> heap_mut	184 bytes smaller, 0.1% faster	64 bytes smaller, 0.88% slower
remove #[cold], no -> heap_mut	152 bytes smaller, 0.1% faster	16 bytes smaller, 0.24% faster

Note that the changes in speed only apply to a vec that needs to be expanded (I benchmarked adding 16 elements to a SmallVec<[u8; 1]>. There is no difference in speed pushing to a vec that never needs to expand.

The second option changes the blocks to

self.reserve_one_unchecked(); 
(ptr, len_ptr) = self.data.heap_mut(); 

since that improves debug builds for some reason, and also looks nicer than the original.

Are either of these worth doing?

[alejandro-vaz]

I'm not sure to be honest

is the juice worth the squeeze??

I wouldn't take anything <1% change in speed as proof of anything

I think we probably have other optimizations that we haven't touched that working on them has a higher ROI

use your judgement on whether they are worth it, and I'd simply say have the one of the three alternatives that makes the codebase consistent and easier to understand

if you think one is worth it, open a PR