flavray / avro-rs

Avro client library implementation in Rust
MIT License
169 stars 95 forks source link

Port benchmarks to criterion #114

Closed cpcloud closed 4 years ago

cpcloud commented 4 years ago

This PR ports micro benchmarks to criterion.

Criterion works on stable, and is actively maintained and developed.

It also provides much more useful benchmarking data than libtest does.

Finally, it keeps track of the last run and gives useful output indicating possible regressions.

poros commented 4 years ago

Uh, this looks pretty cool!

I have never used criterion in my life. Could you past the output of the new cargo bench? Are the benchmark going to run more or less in the same time than before or would they take longer?

cpcloud commented 4 years ago

Criterion is generally slower, the benchmarks take about 20 minutes to run after this PR. However, they are much more reliable and they can be run on stable. Moreover, criterion is actively developed and maintained whereas libtest's benchmarking features are not.

poros commented 4 years ago

Thanks for the explanation. The benchmark are failing to run (we allow failures there because we had problems with clippy in the past and because, you know, nightly). This is the error message:

Benchmarking big schema, read 100k records: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 875.9s or reduce sample count to 10.
Benchmarking big schema, read 100k records: Collecting 100 samples in estimated 875.93 s (5050 iterations)
No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#build-times-out-because-no-output-was-received
The build has been terminated
cpcloud commented 4 years ago

Darn. I guess we can increase the timeout maybe, but long term running benchmarks on some arbitrary cloud instance that is probably container inside a VM is not going to be that useful.

poros commented 4 years ago

Let's increase the timeout for the nightly build a bit. It's true that the actual values won't matter, but at least we can make sure not to break the benchmarks.

poros commented 4 years ago

Can we try prefixing the command with travis_wait 20 and see how it goes before to drop the benchmarks from the CI? I'd love to merge this one, but I feel that trying wouldn't hurt.

cpcloud commented 4 years ago

Yup, I haven't disappeared but am out sick for bit. Will address when I'm back

poros commented 4 years ago
./scripts/travis-test.sh: line 9: travis_wait: command not found

dammit, travis...

cpcloud commented 4 years ago

It looks like it may only be in the shell environment that executes scripts, not in any arbitrary script's environment.

cpcloud commented 4 years ago

I could put a travis_wait 20 call on the entire thing, but that worries me a bit: that could hide some horrible performance bug.

poros commented 4 years ago

OK, fair enough. We tried, let's remove the benchmarks from the nightly tests. Would you mind doing that?

poros commented 4 years ago

Eh, nevermind. Let's merge it and I'll remove it from there. You did enough already :)