-
Notifications
You must be signed in to change notification settings - Fork 122
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
chore: fix criterion benchmarks #206
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea here
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.