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

Add some benchmarks #66

Closed
wants to merge 10 commits into from
Closed

Add some benchmarks #66

wants to merge 10 commits into from

Conversation

Hoverbear
Copy link
Contributor

@Hoverbear Hoverbear commented Jun 12, 2019

This PR adds benchmarks that:

  • Use proptest to generate mildly random values for use.
  • Do microbenchmarking with criterion.
  • Report (with GNUplot) those results.

I'll be adding proptest related tests and more benchmarks as we go, but this is a good scaffold.

Note: I also had to renormalize some line endings.

Some sample results:

test result: ok. 0 passed; 0 failed; 2 ignored; 0 measured; 0 filtered out

     Running target/release/deps/bench-441a949aeb506fc6
raw::put                time:   [2.3470 ms 2.3924 ms 2.4449 ms]
                        change: [-11.100% +1.8898% +17.550%] (p = 0.81 > 0.05)
                        No change in performance detected.
Found 8 outliers among 100 measurements (8.00%)
  3 (3.00%) high mild
  5 (5.00%) high severe

raw::get                time:   [1.3006 ms 1.3138 ms 1.3275 ms]
                        change: [+0.9404% +2.3687% +3.9148%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) high mild

raw::delete             time:   [2.3424 ms 2.3939 ms 2.4549 ms]
                        change: [-1.8414% +1.9432% +5.5014%] (p = 0.30 > 0.05)
                        No change in performance detected.
Found 9 outliers among 100 measurements (9.00%)
  1 (1.00%) low mild
  3 (3.00%) high mild
  5 (5.00%) high severe

raw::batch_put          time:   [3.3776 ms 3.4550 ms 3.5358 ms]
                        change: [-2.3549% +0.9699% +4.7708%] (p = 0.60 > 0.05)
                        No change in performance detected.
Found 10 outliers among 100 measurements (10.00%)
  7 (7.00%) high mild
  3 (3.00%) high severe

raw::batch_get          time:   [1.8608 ms 1.8804 ms 1.9015 ms]
                        change: [-1.5889% -0.0198% +1.5294%] (p = 0.98 > 0.05)
                        No change in performance detected.
Found 6 outliers among 100 measurements (6.00%)
  3 (3.00%) high mild
  3 (3.00%) high severe

raw::batch_delete       time:   [3.1134 ms 3.1646 ms 3.2197 ms]
                        change: [+0.9958% +3.9570% +6.7947%] (p = 0.01 < 0.05)
                        Change within noise threshold.
Found 4 outliers among 100 measurements (4.00%)
  1 (1.00%) low mild
  2 (2.00%) high mild
  1 (1.00%) high severe

@Hoverbear Hoverbear added the enhancement New feature or request label Jun 12, 2019
@Hoverbear Hoverbear requested review from brson and nrc June 12, 2019 00:32
@Hoverbear Hoverbear self-assigned this Jun 12, 2019
Copy link
Collaborator

@nrc nrc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. How reproducible are the results using the randomisation? I'd prefer to use the same input every time we run a benchmark, but we can use random-ish input if there is not too much noise in the results.

@nrc
Copy link
Collaborator

nrc commented Jun 12, 2019

I'd also prefer not to benchmark the roundtrip to the server, and just benchmark the client side of things, but I don't think we can do that yet (or not easily). No harm in having both too.

@Hoverbear
Copy link
Contributor Author

@nrc Yes! non-integration benchmarks are coming, but I wanted to do some full round-trip tests now. :)

The benchmarks are fairly normal. I think you're right that the same sample each run would give us more predictable performance, but we can't really say that performance each run in these kinds of tests will be consistent anyways (there is way too much going on for a slight change in buffer size/values to matter much, and these aren't necessarily clean-slate or configuration-enforced tests, tikv can be configured however).

This also keeps us honest and prevents us from optimizing on specific values.

That said, I have no strong opinions.

@Hoverbear
Copy link
Contributor Author

Maybe I found an ICE?

@nrc
Copy link
Collaborator

nrc commented Jun 13, 2019

I can repro with today's nightly, but not with my previous one, so it's a new bug in the last week or so.

@nrc
Copy link
Collaborator

nrc commented Jun 13, 2019

Looks like rust-lang/rust#61442. It should be fixed by rust-lang/rust#61572 so I think we are better to wait a few days for that to land, rather than work around it.

@jlerche
Copy link

jlerche commented Jun 13, 2019

I think it would be useful to add to the readme how to run the benchmarks. Here's a skeleton blurb how I might structure it:


Running benchmarks

Some of the benchmarks require a connection to a PD and TikV instance. To run them, the addresses of the PD instances must be passed in as environment variable

PD_ADDRS=127.0.0.1:2379 cargo +nightly bench

where PD_ADDRS can also be a comma separated list of addresses to PD instances.

It is possible to specify running specific benchmarks, for example

PD_ADDRS=127.0.0.1:2379 cargo +nightly bench --features integration-tests

@Hoverbear
Copy link
Contributor Author

@jlerche Great catch, added that!

Signed-off-by: Ana Hobden <operator@hoverbear.org>
@Hoverbear Hoverbear mentioned this pull request Jun 18, 2019
Signed-off-by: Ana Hobden <operator@hoverbear.org>
@Hoverbear
Copy link
Contributor Author

I'll base this on #68 since it's a better strategy.

@Hoverbear Hoverbear force-pushed the benches branch 2 times, most recently from b01472e to 33c6964 Compare June 18, 2019 23:52
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
@nrc
Copy link
Collaborator

nrc commented Jul 22, 2019

I'll base this on #68 since it's a better strategy.

ping @Hoverbear are you still planning to rebase this PR?

@Hoverbear
Copy link
Contributor Author

@nrc Yes I'd like to rebase this but I need to get fixtures working first! I think the best way is to use a pre-seeded random this way we don't have to ship data.

@Hoverbear
Copy link
Contributor Author

Nick and I discussed this and we'd like to try another method.

@Hoverbear Hoverbear closed this Jul 30, 2019
@sticnarf sticnarf deleted the benches branch April 7, 2020 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants