chore: fix criterion benchmarks #206
Conversation
Follow up to tokio-rs#151.
|
@Noah-Kennedy This is the kind of output I'm getting from the criterion benchmark with these changes. These times jive with what I was seeing with a different form of no_op benchmark I had experimented with before: that when there is little concurrency, the time per no_op is high, say 3.2 microseconds, but when there are lots of concurrent uring accesses, the amortized time per no_op is very low, about 332 nanoseconds on my slow Linux machine. Running benches/criterion/no_op.rs (target/release/deps/criterion_no_op-22257b96c5b4fa14)
no_op/concurrent tasks/1
time: [3.2306 µs 3.2484 µs 3.2698 µs]
no_op/concurrent tasks/2
time: [1.7880 µs 1.7974 µs 1.8086 µs]
no_op/concurrent tasks/5
time: [877.98 ns 882.60 ns 888.23 ns]
no_op/concurrent tasks/10
time: [574.71 ns 577.88 ns 581.69 ns]
no_op/concurrent tasks/100
time: [324.33 ns 326.25 ns 328.63 ns]
no_op/concurrent tasks/1000
time: [309.72 ns 311.48 ns 313.65 ns]
no_op/concurrent tasks/10000
time: [465.57 ns 469.27 ns 473.69 ns] |
There was a problem hiding this comment.
I like the refactoring and added docs here, but #205 is logically correct and this isn't. I'd recommend pulling some of the changes from that PR into here and then we can just use this PR.
| opts.concurrency = *concurrency; | ||
|
|
||
| // We perform long running benchmarks: this is the best mode | ||
| group.sampling_mode(SamplingMode::Flat); | ||
|
|
||
| group.throughput(Throughput::Elements(opts.iterations as u64)); |
There was a problem hiding this comment.
Don't remove this, set it to be the concurrency value
| let opts = opts.clone(); | ||
| js.spawn_local(async move { | ||
| // Run the required number of iterations (per the concurrency amount) | ||
| for _ in 0..(count / opts.concurrency) { |
There was a problem hiding this comment.
I'm not sure about this approach. If we just use Throughput, criterion will do normalization and adjustment for us.
| js.spawn_local(async move { | ||
| // Run the required number of iterations (per the concurrency amount) | ||
| for _ in 0..(count / opts.concurrency) { | ||
| let _ = black_box(tokio_uring::no_op().await); |
|
I guess you have some issues with how I did it. I will have to pull your branch and see if you get the same kind of results, only better. |
Follow up to #151.