Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Faster insert for the index == len case #282

Merged
merged 2 commits into from Jun 24, 2022

Conversation

nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Jun 23, 2022

This case popped up within rustc, where insert was repeatedly used for a push-like operation. (But in a context where push wasn't always appropriate.) It speeds up rustc on one benchmark by 2% -- not huge, but also not bad for such a small change.

Here is some before and after results for the new benchmark along with the existing push benchmarks.

Old:

test bench_push                        ... bench:         317 ns/iter (+/- 25)
test bench_push_small                  ... bench:          34 ns/iter (+/- 1)
test bench_insert_push                 ... bench:         627 ns/iter (+/- 32)
test bench_insert_push_small           ... bench:          99 ns/iter (+/- 6)

New

test bench_push                        ... bench:         295 ns/iter (+/- 21)
test bench_push_small                  ... bench:          36 ns/iter (+/- 3)
test bench_insert_push                 ... bench:         504 ns/iter (+/- 17)
test bench_insert_push_small           ... bench:          70 ns/iter (+/- 4)

The first two are testing identical code, and show that the time variation is non-trivial. The latter two show the effect of this PR's code changes, which are well beyond the timing variation.

nnethercote added 2 commits Jun 23, 2022
By skipping the call to `copy` with a zero length. This makes it closer
to `push`. This speeds up rustc (which uses `SmallVec` extensively) by
2% on one benchmark.

Also clarify the panic condition.
@nnethercote nnethercote requested a review from mbrubeck Jun 23, 2022
@nnethercote
Copy link
Contributor Author

nnethercote commented Jun 23, 2022

This is the relevant use point within Rust, if you are curious.

@mbrubeck mbrubeck merged commit 7f9e513 into servo:master Jun 24, 2022
4 of 5 checks passed
@mbrubeck
Copy link
Contributor

mbrubeck commented Jun 24, 2022

Thanks! Released as v1.8.1: https://crates.io/crates/smallvec/1.8.1

@nnethercote nnethercote deleted the faster-insert branch Jun 25, 2022
nnethercote added a commit to nnethercote/rust that referenced this issue Jun 26, 2022
This pulls in servo/rust-smallvec#282, which
gives some small wins for rustc.
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 26, 2022
Update `smallvec` to 1.8.1.

This pulls in servo/rust-smallvec#282, which
gives some small wins for rustc.

r? `@lqd`
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 29, 2022
Update `smallvec` to 1.8.1.

This pulls in servo/rust-smallvec#282, which
gives some small wins for rustc.

r? `@lqd`
nnethercote added a commit to nnethercote/rust that referenced this issue Jul 1, 2022
By skipping the call to `copy` with a zero length. This makes it closer
to `push`.

I did this recently for `SmallVec`
(servo/rust-smallvec#282) and it was a big perf win in
one case. Although I don't have a specific use case in mind, it seems
worth doing it for `Vec` as well.

Things to note:
- In the `index < len` case, the number of conditions checked is
  unchanged.
- In the `index == len` case, the number of conditions checked increases
  by one, but the more expensive zero-length copy is avoided.
- In the `index > len` case the code now reserves space for the extra
  element before panicking. This seems like an unimportant change.
bors added a commit to rust-lang-ci/rust that referenced this issue Jul 3, 2022
Optimize `Vec::insert` for the case where `index == len`.

By skipping the call to `copy` with a zero length. This makes it closer
to `push`.

I did this recently for `SmallVec`
(servo/rust-smallvec#282) and it was a big perf win in
one case. Although I don't have a specific use case in mind, it seems
worth doing it for `Vec` as well.

Things to note:
- In the `index < len` case, the number of conditions checked is
  unchanged.
- In the `index == len` case, the number of conditions checked increases
  by one, but the more expensive zero-length copy is avoided.
- In the `index > len` case the code now reserves space for the extra
  element before panicking. This seems like an unimportant change.

r? `@cuviper`
bjorn3 pushed a commit to bjorn3/rustc_codegen_cranelift that referenced this issue Jul 3, 2022
This pulls in servo/rust-smallvec#282, which
gives some small wins for rustc.
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.

None yet

2 participants