Run and collect benchmarks #14556

liyuqian · 2019-12-18T18:55:38Z

This will start to collect engine benchmarks in the flutter-cirrus
Datastore for all post-submit commits.

We're using this to test how https://github.com/liyuqian/metrics_center
works. Once it's stablized, we should move metrics_center into
flutter/packages, and migrate data to flutter-infra Datastore.

Related issue: flutter/flutter#37434

liyuqian · 2019-12-19T00:35:23Z

@cbracken @godofredoc @digiter @keyonghan : I hope that we can start collecting benchmarks for flutter/engine before the end of 2019 :)

liyuqian · 2019-12-19T21:04:22Z

@goderbauer @digiter @keyonghan : I uploaded this first for review because I want this to test https://github.com/liyuqian/metrics_center first. After the testing is satisfied, I'll ask you to review https://github.com/liyuqian/metrics_center , and either merge it into cocoon or flutter/packages.

digiter · 2019-12-20T23:51:38Z

testing/benchmark/bin/parse_and_send.dart

+  if (logResult.exitCode != 0) {
+    throw 'Unexpected exit code ${logResult.exitCode}';
+  }
+  final String gitRevision = logResult.stdout.toString();


Nit: encapsulate the logic of getting git revision into a sub function.

digiter · 2019-12-20T23:55:07Z

testing/benchmark/bin/parse_and_send.dart

+const String kNameKey = 'name';
+const String kTimeUnitKey = 'time_unit';
+
+const List<String> kSkippedSubResults = <String>[


Why skip these sub results?

These are not the numerical values of metrics. Renamed kSkippedSubResults to kNonNumericalValueSubResults for clarity.

digiter · 2019-12-20T23:56:36Z

testing/benchmark/bin/parse_and_send.dart

+        );
+      }
+    }
+  }


Nit: I would also put parsing an item into a sub function.

digiter · 2019-12-20T23:58:12Z

testing/benchmark/bin/parse_and_send.dart

+}
+
+Future<void> main(List<String> args) async {
+  if (args.length != 1) {


Consider using ArgParser()..addFlag, https://dart.dev/tutorials/server/cmdline

I believe addFlag is only for boolean, and I didn't use addOption because I think a simple positional argument is better than a named option here. Also, I didn't find a way to make an option as required using args package... Overall, I found args package to bring more overhead than convenience in this case.

keyonghan · 2019-12-21T00:13:46Z

testing/benchmark/bin/parse_and_send.dart

@@ -0,0 +1,84 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.


The engine repo enforces 2013 through the license check script.

keyonghan · 2019-12-21T00:14:21Z

testing/benchmark/bin/parse_and_send.dart

+
+const String kNameKey = 'name';
+const String kTimeUnitKey = 'time_unit';
+


Nit: remove empty line.

keyonghan · 2019-12-21T00:15:58Z

testing/benchmark/bin/parse_and_send.dart

+
+  final List<FlutterEngineMetricPoint> points = <FlutterEngineMetricPoint>[];
+  for (Map<String, dynamic> item in jsonResult['benchmarks']) {
+    final String name = item[kNameKey];


Nit: remove empty lines between variables, and add one before the for loop?

keyonghan · 2019-12-21T00:17:21Z

testing/benchmark/bin/parse_and_send.dart

+  if (args.length != 1) {
+    throw 'Must have one argument: <benchmark_json_file>';
+  }
+  final List<FlutterEngineMetricPoint> points = await _parse(args[0]);


One check to make sure the file(args[0]) exists before moving forward?

The current code generates the following error when the file does not exist:

Unhandled exception: FileSystemException: Cannot open file, path = 'test.json' (OS Error: No such file or directory, errno = 2)

Does that look good?

keyonghan · 2019-12-21T00:19:27Z

testing/benchmark/bin/parse_and_send.dart

+    throw 'Must have one argument: <benchmark_json_file>';
+  }
+  final List<FlutterEngineMetricPoint> points = await _parse(args[0]);
+  final FlutterDestination destination =


Maybe good to document which DB this link points to.

keyonghan · 2019-12-21T00:19:42Z

testing/benchmark/test/parse_and_send_test.dart

@@ -0,0 +1,20 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.


same here. 2019?

same enforcement of 2013 unfortunately...

digiter

It's unclear to me where and how the data is stored, FlutterDestination seems to define the details.

digiter · 2019-12-21T00:21:57Z

testing/benchmark/pubspec.yaml

+  metrics_center:
+    # TODO(liyuqian): once metrics_center is properly reviewed, add it to
+    # flutter/packages, publish on pub.dev, and use the published package here.
+    git: https://github.com/liyuqian/metrics_center.git


What does metrics center do and why not integrating parse_and_send.dart into metrics center?

Metric center is mainly responsible for extract, transform, and load from some metric database (e.g., cocoon) to some other metric database (e.g., skiaperf). Metric center is itself a database so a client (the engine repo) can directly write metrics into it.

In this case, I intended to only put the client code/logic in the engine repo. That said, I should probably put the transform code (_parse) in the metric center repo, so if some future client also needs to transform https://github.com/google/benchmark results, it doesn't have to rewrite the code.

Thanks for the suggestion!

digiter · 2019-12-21T00:25:58Z

testing/benchmark/test/parse_and_send_test.dart

+void main() {
+  test('parse_and_send with example json does not crash.', () async {
+    final String testCred =
+        File('secret/test_gcp_credentials.json').readAsStringSync();


This "secret" directory is git-ignored, then how could another one run this test?

One should download a service account credentials json from a test GCP project, and put that json as secret/test_gcp_credentials.json. There's a flutter-test project for Flutter team members. I'll add this instruction to the code.

liyuqian

@digiter @keyonghan : thank you for the comments! I've revised the PR according to your suggestions in 7177ad7, and then move the Google benchmark parse logic to metrics center in 2c5968d and liyuqian/metrics_center@af57eb0

liyuqian · 2019-12-22T17:20:45Z

testing/benchmark/bin/parse_and_send.dart

+  if (logResult.exitCode != 0) {
+    throw 'Unexpected exit code ${logResult.exitCode}';
+  }
+  final String gitRevision = logResult.stdout.toString();


liyuqian · 2019-12-22T17:21:23Z

testing/benchmark/bin/parse_and_send.dart

@@ -0,0 +1,84 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.


The engine repo enforces 2013 through the license check script.

liyuqian · 2019-12-22T17:21:41Z

testing/benchmark/bin/parse_and_send.dart

+
+const String kNameKey = 'name';
+const String kTimeUnitKey = 'time_unit';
+


liyuqian · 2019-12-22T17:23:12Z

testing/benchmark/bin/parse_and_send.dart

+const String kNameKey = 'name';
+const String kTimeUnitKey = 'time_unit';
+
+const List<String> kSkippedSubResults = <String>[


These are not the numerical values of metrics. Renamed kSkippedSubResults to kNonNumericalValueSubResults for clarity.

liyuqian · 2019-12-22T17:25:03Z

testing/benchmark/bin/parse_and_send.dart

+
+  final List<FlutterEngineMetricPoint> points = <FlutterEngineMetricPoint>[];
+  for (Map<String, dynamic> item in jsonResult['benchmarks']) {
+    final String name = item[kNameKey];


liyuqian · 2019-12-22T17:39:50Z

testing/benchmark/bin/parse_and_send.dart

+  if (args.length != 1) {
+    throw 'Must have one argument: <benchmark_json_file>';
+  }
+  final List<FlutterEngineMetricPoint> points = await _parse(args[0]);


The current code generates the following error when the file does not exist:

Unhandled exception: FileSystemException: Cannot open file, path = 'test.json' (OS Error: No such file or directory, errno = 2)

Does that look good?

liyuqian · 2019-12-22T17:42:03Z

testing/benchmark/bin/parse_and_send.dart

+    throw 'Must have one argument: <benchmark_json_file>';
+  }
+  final List<FlutterEngineMetricPoint> points = await _parse(args[0]);
+  final FlutterDestination destination =


liyuqian · 2019-12-22T17:49:46Z

testing/benchmark/pubspec.yaml

+  metrics_center:
+    # TODO(liyuqian): once metrics_center is properly reviewed, add it to
+    # flutter/packages, publish on pub.dev, and use the published package here.
+    git: https://github.com/liyuqian/metrics_center.git


Metric center is mainly responsible for extract, transform, and load from some metric database (e.g., cocoon) to some other metric database (e.g., skiaperf). Metric center is itself a database so a client (the engine repo) can directly write metrics into it.

In this case, I intended to only put the client code/logic in the engine repo. That said, I should probably put the transform code (_parse) in the metric center repo, so if some future client also needs to transform https://github.com/google/benchmark results, it doesn't have to rewrite the code.

Thanks for the suggestion!

liyuqian · 2019-12-22T17:50:07Z

testing/benchmark/test/parse_and_send_test.dart

@@ -0,0 +1,20 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.


same enforcement of 2013 unfortunately...

liyuqian · 2019-12-22T17:53:59Z

testing/benchmark/test/parse_and_send_test.dart

+void main() {
+  test('parse_and_send with example json does not crash.', () async {
+    final String testCred =
+        File('secret/test_gcp_credentials.json').readAsStringSync();


One should download a service account credentials json from a test GCP project, and put that json as secret/test_gcp_credentials.json. There's a flutter-test project for Flutter team members. I'll add this instruction to the code.

keyonghan

LGTM

This will start to collect engine benchmarks in the flutter-cirrus Datastore for all post-submit commits. We're using this to test how https://github.com/liyuqian/metrics_center works. Once it's stablized, we should move metrics_center into flutter/packages, and migrate data to flutter-infra Datastore.

liyuqian · 2019-12-24T01:21:44Z

Rerun successful at https://ci.chromium.org/p/flutter/builders/try/Linux%20Host%20Engine/284 . Merging.

flutter/engine@42bb7c9...dc0187f git log 42bb7c9..dc0187f --first-parent --oneline 2019-12-24 [email protected] Roll src/third_party/dart 8736fec094bb..75fc15c7e186 (6 commits) (flutter/engine#14679) 2019-12-24 [email protected] Run and collect benchmarks (flutter/engine#14556) 2019-12-23 [email protected] Roll src/third_party/dart 4a639e8df261..8736fec094bb (11 commits) (flutter/engine#14678) 2019-12-23 [email protected] Roll src/third_party/skia ac29f1342fd2..a2d7225c0fc1 (5 commits) (flutter/engine#14677) 2019-12-23 [email protected] fix selection issue (flutter/engine#14604) 2019-12-23 [email protected] Roll fuchsia/sdk/core/linux-amd64 from TUoVa... to v3R8A... (flutter/engine#14670) 2019-12-23 [email protected] Roll src/third_party/dart cfca5ea9f2e9..4a639e8df261 (7 commits) (flutter/engine#14671) 2019-12-23 [email protected] Roll src/third_party/skia 6ec826085b2a..ac29f1342fd2 (1 commits) (flutter/engine#14672) 2019-12-23 [email protected] Roll fuchsia/sdk/core/mac-amd64 from IZ278... to ZA31z... (flutter/engine#14675) 2019-12-21 [email protected] Roll src/third_party/dart 1db1a837f8c9..cfca5ea9f2e9 (2 commits) (flutter/engine#14667) 2019-12-21 [email protected] Roll src/third_party/skia 2723af6a6d43..6ec826085b2a (3 commits) (flutter/engine#14665) 2019-12-21 [email protected] Roll src/third_party/dart c260e4e8dac6..1db1a837f8c9 (8 commits) (flutter/engine#14606) 2019-12-21 [email protected] Roll src/third_party/skia 28b0c5d4b3bb..2723af6a6d43 (4 commits) (flutter/engine#14605) 2019-12-20 [email protected] View ref pair (flutter/engine#14602) 2019-12-20 [email protected] Roll fuchsia/sdk/core/mac-amd64 from VC7eE... to IZ278... (flutter/engine#14603) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

flutter/engine@5a730c6...bdc9708 git log 5a730c6..bdc9708 --first-parent --oneline 2019-12-28 [email protected] Revert "Use ELF for Dart AOT snapshots on Fuchsia. (#13896)" (flutter/engine#14823) 2019-12-27 [email protected] Roll src/third_party/skia 89bf1547f3aa..5a2f962313a5 (16 commits) (flutter/engine#14825) 2019-12-27 [email protected] Roll fuchsia/sdk/core/linux-amd64 from ikxBh... to sxapd... (flutter/engine#14821) 2019-12-27 [email protected] Roll fuchsia/sdk/core/mac-amd64 from IEPKx... to iImpF... (flutter/engine#14817) 2019-12-27 [email protected] Roll src/third_party/dart 94a4f6415e6c..c547f5d933e5 (9 commits) (flutter/engine#14812) 2019-12-27 [email protected] Roll src/third_party/skia f2d522a3f434..89bf1547f3aa (4 commits) (flutter/engine#14814) 2019-12-26 [email protected] Roll src/third_party/dart 232a171a0b1c..94a4f6415e6c (8 commits) (flutter/engine#14808) 2019-12-26 [email protected] Roll src/third_party/skia 07d744fb8a53..f2d522a3f434 (4 commits) (flutter/engine#14807) 2019-12-26 [email protected] Roll src/third_party/skia 05eb83be171c..07d744fb8a53 (3 commits) (flutter/engine#14806) 2019-12-26 [email protected] Roll fuchsia/sdk/core/linux-amd64 from v3R8A... to ikxBh... (flutter/engine#14802) 2019-12-26 [email protected] Roll src/third_party/dart 75fc15c7e186..232a171a0b1c (4 commits) (flutter/engine#14803) 2019-12-26 [email protected] Roll fuchsia/sdk/core/mac-amd64 from ZA31z... to IEPKx... (flutter/engine#14702) 2019-12-26 [email protected] Roll src/third_party/skia 3d3150c89c5f..05eb83be171c (3 commits) (flutter/engine#14800) 2019-12-24 [email protected] Roll src/third_party/skia a2d7225c0fc1..3d3150c89c5f (1 commits) (flutter/engine#14680) 2019-12-24 [email protected] Roll src/third_party/dart 8736fec094bb..75fc15c7e186 (6 commits) (flutter/engine#14679) 2019-12-24 [email protected] Run and collect benchmarks (flutter/engine#14556) 2019-12-23 [email protected] Roll src/third_party/dart 4a639e8df261..8736fec094bb (11 commits) (flutter/engine#14678) 2019-12-23 [email protected] Roll src/third_party/skia ac29f1342fd2..a2d7225c0fc1 (5 commits) (flutter/engine#14677) 2019-12-23 [email protected] fix selection issue (flutter/engine#14604) 2019-12-23 [email protected] Roll fuchsia/sdk/core/linux-amd64 from TUoVa... to v3R8A... (flutter/engine#14670) 2019-12-23 [email protected] Roll src/third_party/dart cfca5ea9f2e9..4a639e8df261 (7 commits) (flutter/engine#14671) 2019-12-23 [email protected] Roll src/third_party/skia 6ec826085b2a..ac29f1342fd2 (1 commits) (flutter/engine#14672) 2019-12-23 [email protected] Roll fuchsia/sdk/core/mac-amd64 from IZ278... to ZA31z... (flutter/engine#14675) 2019-12-21 [email protected] Roll src/third_party/dart 1db1a837f8c9..cfca5ea9f2e9 (2 commits) (flutter/engine#14667) 2019-12-21 [email protected] Roll src/third_party/skia 2723af6a6d43..6ec826085b2a (3 commits) (flutter/engine#14665) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

This will start to collect engine benchmarks in the flutter-cirrus Datastore for all post-submit commits. We're using this to test how https://github.com/liyuqian/metrics_center works. Once it's stablized, we should move metrics_center into flutter/packages, and migrate data to flutter-infra Datastore. Related issue: flutter/flutter#37434

auto-assign bot requested a review from cbracken December 18, 2019 18:55

googlebot added cla: yes labels Dec 18, 2019

liyuqian force-pushed the benchmark branch 12 times, most recently from b64b1f8 to 31b2369 Compare December 19, 2019 00:27

liyuqian changed the title ~~[wip] Run and collect benchmarks~~ Run and collect benchmarks Dec 19, 2019

liyuqian requested review from keyonghan, digiter and godofredoc December 19, 2019 00:30

liyuqian force-pushed the benchmark branch from 31b2369 to d067971 Compare December 19, 2019 00:33

liyuqian force-pushed the benchmark branch 2 times, most recently from 57729b8 to 34843b4 Compare December 19, 2019 23:43

digiter reviewed Dec 20, 2019

View reviewed changes

keyonghan reviewed Dec 21, 2019

View reviewed changes

digiter reviewed Dec 21, 2019

View reviewed changes

liyuqian commented Dec 22, 2019

View reviewed changes

keyonghan approved these changes Dec 23, 2019

View reviewed changes

liyuqian added 3 commits December 23, 2019 11:58

Fixes

e6dc049

Move the Google benchmark parse to metric center

84a66c8

liyuqian force-pushed the benchmark branch from 2c5968d to 84a66c8 Compare December 23, 2019 19:58

liyuqian merged commit 17fa0fa into flutter:master Dec 24, 2019

engine-flutter-autoroll mentioned this pull request Dec 26, 2019

Roll engine 42bb7c9eea9a..dc0187f21777 (15 commits) flutter/flutter#47827

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 26, 2019

17fa0fa Run and collect benchmarks (flutter/engine#14556)

00a8f7f

engine-flutter-autoroll mentioned this pull request Dec 26, 2019

Roll engine 42bb7c9eea9a..dc0187f21777 (15 commits) flutter/flutter#47828

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 26, 2019

17fa0fa Run and collect benchmarks (flutter/engine#14556)

2ae5d73

engine-flutter-autoroll mentioned this pull request Dec 26, 2019

Roll engine 42bb7c9eea9a..dc0187f21777 (15 commits) flutter/flutter#47829

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 26, 2019

17fa0fa Run and collect benchmarks (flutter/engine#14556)

5c2b212

engine-flutter-autoroll mentioned this pull request Dec 28, 2019

Roll engine 5a730c60d3ce..bdc9708d235e (25 commits) flutter/flutter#47926

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 28, 2019

17fa0fa Run and collect benchmarks (flutter/engine#14556)

c489a31

liyuqian deleted the benchmark branch March 2, 2020 19:21

liyuqian mentioned this pull request Jan 13, 2021

Run engine benchmarks and track in our dashboard flutter/flutter#34746

Closed

		@@ -0,0 +1,84 @@
		// Copyright 2013 The Flutter Authors. All rights reserved.


		const String kNameKey = 'name';
		const String kTimeUnitKey = 'time_unit';

		@@ -0,0 +1,20 @@
		// Copyright 2013 The Flutter Authors. All rights reserved.

Run and collect benchmarks #14556

Run and collect benchmarks #14556

Uh oh!

Conversation

liyuqian commented Dec 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liyuqian commented Dec 19, 2019

Uh oh!

liyuqian commented Dec 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keyonghan Dec 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

digiter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liyuqian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

liyuqian commented Dec 18, 2019 •

edited

Loading

keyonghan Dec 21, 2019 •

edited

Loading