Add AsyncBenchmarkBase class #44

aam · 2019-08-30T18:21:37Z

This is to accommodate tests that require async setup/teardown/run.

sortie · 2019-09-05T11:54:22Z

Hi Kevin, I'd like to take over ownership of this package as we're now using it to benchmark the Dart SDK. Can you add me as an uploader for the https://pub.dev/packages/benchmark_harness package?

sortie · 2019-09-05T13:20:22Z

lib/src/benchmark_base.dart

+  run() async {}
+
+  // Runs a short version of the benchmark. By default invokes [run] once.
+  void warmup() async {


Future, and likewise above and the other places

sortie · 2019-09-05T13:33:48Z

lib/src/benchmark_base.dart

+    for (int i = 0; i < 10; i++) {
+      await run();
+    }
+  }


Okay story time. The class in package:benchmark_harness is bug compatible with Golem and I need to keep them that way until I've migrated the benchmarks. Golem has an existing AsyncBenchmarkBase, and I need to keep this version compatible with it.

BenchmarkBase (including the one here) has a bug where it does ten iterations per exercise, but it doesn't multiply the final result by 10, meaning that it reports the time to run 10 iterations instead of 1. The documentation for this package mentions that.

The existing AsyncBenchmarkBase doesn't have that problem. It does one run per exercise. I need this version to be compatible with that. That's still not great because doing 10 iterations per exercise means less overhead checking the system time, but at least the results aren't misleading.

My intention is to take over this package and ensure there's a pair of sync/async base benchmark classes that report the time per iteration, and perhaps does 10 iterations per exercise.

So where does that leave you? I do need a class named AsyncBenchmarkBase that does one run per exercise and reports the time per iteration. This is results compatible with how we want things to be in the future. You can use that base class in your Isolates benchmark. When I make changes to this package in the future, I can update your benchmark while being results compatible.

sortie · 2019-09-05T13:34:07Z

lib/src/benchmark_base.dart

@@ -62,3 +62,64 @@ class BenchmarkBase {
    emitter.emit(name, measure());
  }
 }
+
+class AsyncBenchmarkBase {


Put this in its own async_benchmark_base.dart file.

sortie · 2019-09-05T13:35:03Z

lib/src/benchmark_base.dart

+  // Measures the score for this benchmark by executing it repeately until
+  // time minimum has been reached.
+  static Future<double> measureFor(Function f, int minimumMillis) async {
+    int minimumMicros = minimumMillis * 1000;


Use final for variables that are assigned once.

sortie · 2019-09-05T13:35:26Z

lib/src/benchmark_base.dart

+    await measureFor(warmup, 100);
+    // Run the benchmark for at least 2000ms.
+    double result = await measureFor(exercise, 2000);
+    await teardown();


Put this in a finally block. That's another difference between BenchmarkBase and AsyncBenchmarkBase.

Fox32 · 2020-02-28T17:26:48Z

Just wanted to submit a PR like this, but it's already here. Can this be merged?

davidmartos96 · 2020-07-28T09:01:30Z

@sortie Any update on this? I think the only thing missing from the code review would be to replace the exercise function with the following so that it behaves the same as the sync version.

Future<void> exercise() async {
  for (int i = 0; i < 10; i++) {
    await run();
  }
}

I think the async version of the benchmarks can be very helpful for comparing things like database performance. It would be great if this could be merged at some point.

sortie · 2020-07-28T14:12:00Z

Yes, the plan is to do this work soon. package:benchmark_harness is currently central to the ongoing null safety benchmark migration and has been marked as null safe in master, but not published on pub of course. That means things are a bit in flux right now. In the meanwhile, I want to add some features such as this async harness to the package in order to finish the migration, and I also want to make some breaking changes. So overall, this work needs to be done in a thought out manner and this change will have to wait just a bit longer.

mkustermann · 2022-04-26T08:57:13Z

If the only thing blocking this is the discussion about whether the number of iterations from the exercise function is taken into account for the reporting, then here's a suggestion: The harness class could take in it's constructor a boolean parameter signaling which behavior we want (or an integer parameter specifying the iteration count).

Apart from that, is there anything else to be decided before we can move forward here?

sortie · 2022-04-26T18:54:54Z

lib/src/async_benchmark_base.dart

+  final String name;
+  final ScoreEmitter emitter;
+
+  // Empty constructor.


Switch the comments to triple slash dartdoc for documentation

sortie · 2022-04-26T18:56:32Z

lib/src/async_benchmark_base.dart

+  const AsyncBenchmarkBase(this.name, {this.emitter = const PrintEmitter()});
+
+  // The benchmark code.
+  // This function is not used, if both [warmup] and [exercise] are overwritten.


Blank dartdoc line between the first paragraph and the second per style.

sortie · 2022-04-26T19:00:33Z

Hi Martin,

First up, I need to stabilize this package as it hasn't rolled into the Dart SDK in a long time. There's a slight risk that any of the changes in the meanwhile may have broken something. That roll is being done in https://dart-review.googlesource.com/c/sdk/+/212747

The PR has already been updated to be compatible with the historic AsyncBenchmarkBase behavior (which happens to be the intended behavior). I see a couple minor nits. I'll take a careful look once the benchmark_harness roll goes through and probably just take over this CL and get it fully done.

Jonas

mkustermann · 2022-04-27T08:23:29Z

Thanks for getting the ball moving here again, @sortie (and thanks to @aam for doing the change in the first place :))!

The async benchmark harness accommodates tests that require async setup/teardown/run implementations.

sortie

OK I took over this PR and got it over the finish line :)

…sync Add AsyncBenchmarkBase class

googlebot added the cla: yes label Aug 30, 2019

aam requested a review from kevmoo August 30, 2019 18:23

sortie requested review from sortie and removed request for kevmoo September 5, 2019 11:34

sortie suggested changes Sep 5, 2019

View reviewed changes

unsuitable001 mentioned this pull request Jul 7, 2021

Benchmarks google/cronet.dart#3

Closed

sortie suggested changes Apr 26, 2022

View reviewed changes

sortie force-pushed the async branch from 65e31e4 to 396cd47 Compare April 29, 2022 11:47

Add AsyncBenchmarkBase.

45013a8

The async benchmark harness accommodates tests that require async setup/teardown/run implementations.

sortie force-pushed the async branch from 396cd47 to 45013a8 Compare April 29, 2022 11:51

sortie approved these changes Apr 29, 2022

View reviewed changes

sortie merged commit 0530da6 into master Apr 29, 2022

sortie deleted the async branch April 29, 2022 11:55

mosuem pushed a commit to dart-lang/tools that referenced this pull request Oct 25, 2024

Merge pull request dart-archive/benchmark_harness#44 from dart-lang/a…

43c1085

…sync Add AsyncBenchmarkBase class

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AsyncBenchmarkBase class #44

Add AsyncBenchmarkBase class #44

aam commented Aug 30, 2019

sortie commented Sep 5, 2019

sortie Sep 5, 2019

aam Sep 5, 2019

sortie Sep 5, 2019

aam Sep 5, 2019

sortie Sep 5, 2019

aam Sep 5, 2019

sortie Sep 5, 2019

aam Sep 5, 2019

sortie Sep 5, 2019

aam Sep 5, 2019

Fox32 commented Feb 28, 2020

davidmartos96 commented Jul 28, 2020

sortie commented Jul 28, 2020

mkustermann commented Apr 26, 2022

sortie Apr 26, 2022

sortie Apr 26, 2022

sortie commented Apr 26, 2022

mkustermann commented Apr 27, 2022

sortie left a comment

Add AsyncBenchmarkBase class #44

Add AsyncBenchmarkBase class #44

Conversation

aam commented Aug 30, 2019

sortie commented Sep 5, 2019

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

Choose a reason for hiding this comment

Fox32 commented Feb 28, 2020

davidmartos96 commented Jul 28, 2020

sortie commented Jul 28, 2020

mkustermann commented Apr 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sortie commented Apr 26, 2022

mkustermann commented Apr 27, 2022

sortie left a comment

Choose a reason for hiding this comment