use criterion #5981

maminrayej · 2023-09-04T09:58:20Z

Replaces bencher with criterion.

Resolves #5979.

wathenjiang · 2023-09-04T14:00:42Z

Thanks maminrayej, your work helps a lot.

wathenjiang · 2023-09-04T14:07:26Z

benches/signal.rs

    });
 }

-benchmark_group!(signal_group, many_signals,);
+criterion_group!(signal_group, many_signals,);


The commas in these macros call can be deleted.

wathenjiang · 2023-09-04T14:07:44Z

benches/time_now.rs

 }

-bencher::benchmark_group!(time_now, time_now_current_thread,);
+criterion_group!(time_now, time_now_current_thread,);


The commas in these macros call can be deleted.

wathenjiang · 2023-09-04T15:13:03Z

benches/sync_notify.rs

    });
 }

-bencher::benchmark_group!(
+criterion_group!(
    notify_waiters_simple,


With criterion crate, we can group the similar benchmarks in one group.
Let me use the benches/sync_notify.rs as example to expalin this.

The notify_one::<10>、notify_one::<100>..., they almost same, but only the N_WAITERS is not equal.

So, we can group these benchmarks:

use std::sync::Arc; use std::sync::atomic::{AtomicUsize, Ordering}; use criterion::{BenchmarkGroup, Criterion, criterion_group, criterion_main}; use criterion::measurement::WallTime; use tokio::sync::Notify; fn rt() -> tokio::runtime::Runtime { tokio::runtime::Builder::new_multi_thread() .worker_threads(6) .build() .unwrap() } fn notify_waiters<const N_WAITERS: usize>(group: &mut BenchmarkGroup<WallTime>) { let rt = rt(); let notify = Arc::new(Notify::new()); let counter = Arc::new(AtomicUsize::new(0)); for _ in 0..N_WAITERS { rt.spawn({ let notify = notify.clone(); let counter = counter.clone(); async move { loop { notify.notified().await; counter.fetch_add(1, Ordering::Relaxed); } } }); } const N_ITERS: usize = 500; group.bench_function(N_WAITERS.to_string(), |b| b.iter(|| { counter.store(0, Ordering::Relaxed); loop { notify.notify_waiters(); if counter.load(Ordering::Relaxed) >= N_ITERS { break; } } } )); } fn notify_one<const N_WAITERS: usize>(group: &mut BenchmarkGroup<WallTime>) { let rt = rt(); let notify = Arc::new(Notify::new()); let counter = Arc::new(AtomicUsize::new(0)); for _ in 0..N_WAITERS { rt.spawn({ let notify = notify.clone(); let counter = counter.clone(); async move { loop { notify.notified().await; counter.fetch_add(1, Ordering::Relaxed); } } }); } const N_ITERS: usize = 500; group.bench_function(N_WAITERS.to_string(), |b| b.iter(|| { counter.store(0, Ordering::Relaxed); loop { notify.notify_one(); if counter.load(Ordering::Relaxed) >= N_ITERS { break; } } } )); } fn bench_notify_waiters(c: &mut Criterion) { let mut group = c.benchmark_group("notify_waiters"); notify_waiters::<10>(&mut group); notify_waiters::<50>(&mut group); notify_waiters::<100>(&mut group); notify_waiters::<200>(&mut group); notify_waiters::<500>(&mut group); group.finish(); } fn bench_notify_one(c: &mut Criterion) { let mut group = c.benchmark_group("notify_one"); notify_one::<10>(&mut group); notify_one::<50>(&mut group); notify_one::<100>(&mut group); notify_one::<200>(&mut group); notify_one::<500>(&mut group); group.finish(); } criterion_group!( notify_waiters_simple, bench_notify_waiters, bench_notify_one, ); criterion_main!(notify_waiters_simple);

The benchmark output is the following:

notify_waiters/10 time: [136.88 µs 142.21 µs 148.36 µs] change: [+4.8894% +7.8281% +11.130%] (p = 0.00 < 0.05) Performance has regressed. Found 16 outliers among 100 measurements (16.00%) 3 (3.00%) high mild 13 (13.00%) high severe notify_waiters/50 time: [128.05 µs 129.35 µs 130.75 µs] change: [+1.4689% +3.0748% +4.6824%] (p = 0.00 < 0.05) Performance has regressed. Found 4 outliers among 100 measurements (4.00%) 3 (3.00%) high mild 1 (1.00%) high severe notify_waiters/100 time: [119.49 µs 120.84 µs 122.25 µs] change: [-3.9765% -2.1273% +0.0038%] (p = 0.04 < 0.05) Change within noise threshold. ....

Does it is more readable than current version output:

notify_waiters time: [129.83 µs 131.15 µs 132.53 µs] change: [-7.7179% -4.5238% -1.1667%] (p = 0.01 < 0.05) Performance has improved. Found 8 outliers among 100 measurements (8.00%) 4 (4.00%) high mild 4 (4.00%) high severe notify_waiters #2 time: [131.63 µs 133.83 µs 136.14 µs] change: [-3.7356% -1.2708% +0.8673%] (p = 0.31 > 0.05) No change in performance detected. Found 5 outliers among 100 measurements (5.00%) 4 (4.00%) high mild 1 (1.00%) high severe notify_waiters #3 time: [130.58 µs 134.51 µs 139.47 µs] change: [+16.498% +26.125% +36.543%] (p = 0.00 < 0.05) Performance has regressed. ....

I agree. is there anywhere else in the benchmarks you would like to see them grouped?

I belive the benchmarks of components in sync mod can be grouped, including the sync_mpsc.rs, sync_rwlock.rs, sync_semaphore.rs. They might can be grouped by contention or uncontention, new_multi_thread or new_current_thread, and full or unfull. But I dont want them to be grouped too complicatedly, grouping them once might be enough.

And could we use generics to simplify the benchmark like sync_notify.rs does?

I tried to use generics where it made sense and grouped benchmarks together by contented or uncontented.

wathenjiang · 2023-09-08T13:13:00Z

benches/sync_mpsc.rs

+fn group_send(c: &mut Criterion) {
+    let mut group = c.benchmark_group("send");
+    send_data::<Medium, 1000>(&mut group, "Medium");
+    send_data::<Large, 1000>(&mut group, "Medium");


We are not allowed use a same name in the one benchmark group. Do you mean "Large" here?
Wouldn't it be better for us to use lowercase words here, "medium" and "large"?

wathenjiang · 2023-09-08T13:16:38Z

benches/sync_mpsc.rs

+    group.finish();
+}
+
+criterion_group!(create, group_create_medium);


I belive it's better that the function here starts wich bench_ instead of group_(group_create_medium to bench_create_medium ). Those functions are orginized by group, but they are benchmark function.

wathenjiang · 2023-09-08T13:32:52Z

benches/sync_semaphore.rs

@@ -28,14 +29,14 @@ async fn task(s: Arc<Semaphore>) {
    drop(permit);
 }

-fn uncontended_concurrent_multi(c: &mut Criterion) {
+fn uncontended_concurrent_multi(g: &mut BenchmarkGroup<WallTime>) {
    let rt = tokio::runtime::Builder::new_multi_thread()


I found we call the following code many times：

let rt = tokio::runtime::Builder::new_multi_thread() .worker_threads(6) .build() .unwrap();

let rt = tokio::runtime::Builder::new_current_thread() .build() .unwrap();

Would it be better to write two constructor functions for them:

fn single_rt() -> Runtime{ tokio::runtime::Builder::new_current_thread().build().unwrap() }

and

fn multi_rt() -> Runtime{ tokio::runtime::Builder::new_multi_thread() .worker_threads(6) .build() .unwrap() }

maminrayej · 2023-09-08T18:21:58Z

@wathenjiang done :)

wathenjiang · 2023-09-09T07:39:23Z

benches/sync_mpsc_oneshot.rs

 }

-fn request_reply(b: &mut Bencher, rt: Runtime) {
+fn request_reply(b: &mut Criterion, rt: Runtime) {


Would it be better to make the request_reply has a name of runtime, such as muilti_thread and current_thread? Or we got request_reply and request_reply #2, they are not very clear name of benchmark test.

I focused primarily on replacing bencher with criterion in this PR. I've attempted to incorporate some of the clean-up suggestions you provided, but I'd like to concentrate on other issues at the moment. If there are additional clean-ups you'd like to see, it might be beneficial to address them in a subsequent PR.

I am ok with it. This PR looks good to me. Thanks.
@Darksonn could you review this?

Darksonn

This looks fine to me. Please confirm that you have run all of the benchmarks and that the output looks reasonable. Then, please merge master into this branch (you can just click the button on this page). Once you've done that, I'm happy to merge this.

maminrayej · 2023-09-10T10:01:59Z

I have run the benchmarks locally, and the results appear to be reasonable.

Darksonn · 2023-09-10T14:45:18Z

Thanks!

use criterion

7414355

Darksonn added the A-benches Area: Benchmarks label Sep 4, 2023

wathenjiang reviewed Sep 4, 2023

View reviewed changes

group benchmarks

9760b7c

wathenjiang reviewed Sep 8, 2023

View reviewed changes

resolve comments

4bce885

wathenjiang reviewed Sep 9, 2023

View reviewed changes

Darksonn approved these changes Sep 10, 2023

View reviewed changes

Merge branch 'master' into use-criterion

6413f96

Darksonn merged commit b046c0d into tokio-rs:master Sep 10, 2023
70 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use criterion #5981

use criterion #5981

maminrayej commented Sep 4, 2023

wathenjiang commented Sep 4, 2023

wathenjiang Sep 4, 2023

wathenjiang Sep 4, 2023

wathenjiang Sep 4, 2023

maminrayej Sep 5, 2023

wathenjiang Sep 6, 2023

maminrayej Sep 6, 2023

wathenjiang Sep 8, 2023

wathenjiang Sep 8, 2023

wathenjiang Sep 8, 2023

maminrayej commented Sep 8, 2023

wathenjiang Sep 9, 2023

maminrayej Sep 9, 2023

wathenjiang Sep 9, 2023

Darksonn left a comment

maminrayej commented Sep 10, 2023

Darksonn commented Sep 10, 2023

use criterion #5981

use criterion #5981

Conversation

maminrayej commented Sep 4, 2023

wathenjiang commented Sep 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maminrayej commented Sep 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Darksonn left a comment

Choose a reason for hiding this comment

maminrayej commented Sep 10, 2023

Darksonn commented Sep 10, 2023