Skip to content

Move benchmarks to benches module with noinline wrappers #32

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

Merged
merged 2 commits into from
Sep 14, 2016

Conversation

nipunn1313
Copy link
Contributor

@nipunn1313 nipunn1313 commented Sep 12, 2016

This ensures that the benchmarks are in a separate crate and linked
against the smallvec dynamic library rather than being compiled
together.

It also nicely removes the need for the "benchmarks" feature.


This change is Reviewable

This ensures that the benchmarks are in a separate crate and linked
against the smallvec dynamic library rather than being compiled
together.
@nipunn1313
Copy link
Contributor Author

nipunn1313 commented Sep 12, 2016

Used a #noinline wrapper from benchmarks to get more consistent results.
Before/after shown here.

Looks like the results are a bit worse, but more consistent/realistic since the compiler's inlining during the benchmark itself won't affect the performance.

Nipunns-MBP:rust-smallvec nipunn$ git checkout master ; cargo bench --features benchmarks bench ; git checkout benchcrate ; cargo bench bench
Switched to branch 'master'
Your branch is up-to-date with 'origin/master'.
   Compiling smallvec v0.2.0 (file:///Users/nipunn/src/rust-smallvec)
    Finished release [optimized] target(s) in 2.40 secs
     Running target/release/deps/smallvec-a59689f47ae774cb

running 4 tests
test bench::bench_extend  ... bench:         154 ns/iter (+/- 22)
test bench::bench_insert  ... bench:       1,144 ns/iter (+/- 520)
test bench::bench_push    ... bench:         277 ns/iter (+/- 74)
test bench::bench_pushpop ... bench:         352 ns/iter (+/- 34)

test result: ok. 0 passed; 0 failed; 0 ignored; 4 measured

Switched to branch 'benchcrate'
   Compiling smallvec v0.2.0 (file:///Users/nipunn/src/rust-smallvec)
    Finished release [optimized] target(s) in 2.28 secs
     Running target/release/bench-ef222b4531a86f9a

running 4 tests
test bench_extend  ... bench:         146 ns/iter (+/- 24)
test bench_insert  ... bench:       1,324 ns/iter (+/- 209)
test bench_push    ... bench:         439 ns/iter (+/- 170)
test bench_pushpop ... bench:         360 ns/iter (+/- 218)

test result: ok. 0 passed; 0 failed; 0 ignored; 4 measured

     Running target/release/deps/smallvec-a59689f47ae774cb

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured

@nipunn1313 nipunn1313 changed the title Move benchmarks to benches module Move benchmarks to benches module with noinline wrappers Sep 12, 2016
@nipunn1313
Copy link
Contributor Author

nipunn1313 commented Sep 12, 2016

I verified by disassembling the generated executables that beforehand, insert() was being inlined, but after this change it is not. This accounts for the dip in the bench_insert

@nipunn1313
Copy link
Contributor Author

Ping on these improvements to the benchmarks.

@jdm
Copy link
Member

jdm commented Sep 14, 2016

@bors-servo: r+
Seems reasonable! Thanks!

@bors-servo
Copy link
Contributor

📌 Commit 59f5ea6 has been approved by jdm

@bors-servo
Copy link
Contributor

⚡ Test exempted - status

@bors-servo bors-servo merged commit 59f5ea6 into servo:master Sep 14, 2016
bors-servo pushed a commit that referenced this pull request Sep 14, 2016
Move benchmarks to benches module with noinline wrappers

This ensures that the benchmarks are in a separate crate and linked
against the smallvec dynamic library rather than being compiled
together.

It also nicely removes the need for the "benchmarks" feature.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-smallvec/32)
<!-- Reviewable:end -->
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.

3 participants