Skip to content

Commit

Permalink
std: Avoid ptr::copy if unnecessary in vec::Drain
Browse files Browse the repository at this point in the history
This commit is spawned out of a performance regression investigation in #50496.
In tracking down this regression it turned out that the `expand_statements`
function in the compiler was taking quite a long time. Further investigation
showed two key properties:

* The function was "fast" on glibc 2.24 and slow on glibc 2.23
* The hottest function was memmove from glibc

Combined together it looked like glibc gained an optimization to the memmove
function in 2.24. Ideally we don't want to rely on this optimization, so I
wanted to dig further to see what was happening.

The hottest part of `expand_statements` was `Drop for Drain` in the call to
`splice` where we insert new statements into the original vector. This *should*
be a cheap operation because we're draining and replacing iterators of the exact
same length, but under the hood memmove was being called a lot, causing a
slowdown on glibc 2.23.

It turns out that at least one of the optimizations in glibc 2.24 was that
`memmove` where the src/dst are equal becomes much faster. [This program][prog]
executes in ~2.5s against glibc 2.23 and ~0.3s against glibc 2.24, exhibiting
how glibc 2.24 is optimizing `memmove` if the src/dst are equal.

And all that brings us to what this commit itself is doing. The change here is
purely to `Drop for Drain` to avoid the call to `ptr::copy` if the region being
copied doesn't actually need to be copied. For normal usage of just `Drain`
itself this check isn't really necessary, but because `Splice` internally
contains `Drain` this provides a nice speed boost on glibc 2.23. Overall this
should fix the regression seen in #50496 on glibc 2.23 and also fix the
regression on Windows where `memmove` looks to not have this optimization.

Note that the way `splice` was called in `expand_statements` would cause a
quadratic number of elements to be copied via `memmove` which is likely why the
tuple-stress benchmark showed such a severe regression.

Closes #50496

[prog]: https://gist.github.com/alexcrichton/c05bc51c6771bba5ae5b57561a6c1cd3
  • Loading branch information
alexcrichton committed May 9, 2018
1 parent 8ff4b42 commit 254b601
Showing 1 changed file with 5 additions and 3 deletions.
8 changes: 5 additions & 3 deletions src/liballoc/vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2533,9 +2533,11 @@ impl<'a, T> Drop for Drain<'a, T> {
// memmove back untouched tail, update to new length
let start = source_vec.len();
let tail = self.tail_start;
let src = source_vec.as_ptr().offset(tail as isize);
let dst = source_vec.as_mut_ptr().offset(start as isize);
ptr::copy(src, dst, self.tail_len);
if tail != start {
let src = source_vec.as_ptr().offset(tail as isize);
let dst = source_vec.as_mut_ptr().offset(start as isize);
ptr::copy(src, dst, self.tail_len);
}
source_vec.set_len(start + self.tail_len);
}
}
Expand Down

0 comments on commit 254b601

Please sign in to comment.