Added performance benchmarks for platform channels #81414

gaaclarke · 2021-04-28T19:57:16Z

As outlined in the doc: https://docs.google.com/document/d/1oNLxJr_ZqjENVhF94-PqxsGPx0qGXx-pRJxXL6LSagc/edit

This just adds tests for iOS and Android for BasicMessageChannels with the StandardMessageCodec.

issue: #81559

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

goderbauer

There's a lot of boilerplate in this PR, I hope I reviewed all the right files...

Would probably be good if we found a way to not duplicate all this boilerplate for different benchmarks.

dev/benchmarks/platform_channels_benchmarks/.gitignore

goderbauer · 2021-04-29T16:35:20Z

dev/benchmarks/platform_channels_benchmarks/lib/main.dart

+}
+
+void main() {
+  runApp(const MyApp());


Do you actually need the Counter app UI? To simplify, could this just be:

void main() { WidgetsFlutterBinding.ensureInitialized(); _runTests(); }

That doesn't work because that doesn't setup the binary messenger for platform channels. I've removed most of the UI.

That's supposed to set it up though, right?

If not, can we just directly do what's needed to set up the default binary messenger?

It doesn't say anything about that in the documentation. What's the concern? My understanding is that goderbauer didn't want all this extra code, it's mostly been removed. If we had a PlatformChannelsBinding.ensureInitialized() function call it would save maybe 6 lines of code if I paired down the UI even further. I imagine things might get a bit goofy if you don't present any widget though. Do you want me to trim even more away?

For me it's more that it takes that much longer to find out what this test is actually doing from here. I have to read through a bunch of unrelated widgets to finally get to one that calls a method in initState that has nothing to do with the widget itself. I also have to think a little bit about whether that could have any impact on the benchmark.

I'm kind of surprised the WidgetsFlutterBinding.ensureInitialized isn't enough to get the channels set up - that's what we tell people to do if they need to talk to platform channels before any UI is rendered after all.

This test doesn't really need any UI to run - it'd just show a black screen in that case. If that bothers you, you can just have it show a Placeholder widget, or maybe some text saying it's running the benchmark.

And in case it's not clear, IMO this shouldn't block the test from landing. But future us will probably be grateful for any simplifications that can be made now :)

I think we should remove everything from the benchmark that's not required for it to make it clear what this is testing. Since this benchmark doesn't rely on any UI, why show it? If you really need to show UI, then just show a simple container.

Okay, I removed everything I could. It just shows a black screen now while executing.

dev/benchmarks/platform_channels_benchmarks/lib/main.dart

goderbauer · 2021-04-29T16:38:24Z

dev/benchmarks/platform_channels_benchmarks/lib/main.dart

+  final List<Object> largeBuffer = _makeTestBuffer(1000);
+  const StandardMessageCodec codec = StandardMessageCodec();
+  final ByteData data = codec.encodeMessage(largeBuffer);
+  print('Large buffer size: ${data.lengthInBytes}');


remove the print?

This was intentional to leave this in for the logs since it's difficult to make the datastructure of a specific size and it's important that its size is known in order to make valid comparisons. I might actually tweak the size before I turn the tests on. I gotta check to make sure this is what we want to test.

Can we somehow capture this in the benchmark metric? Like, maybe make it us/byte or something? Or just include the number of bytes processed somewhere in the metrics?

Even if we capture us/byte changing the size of the buffer in the test would create a change in us/byte, it won't scale linearly (byte bytes are copied 4 different times and serializing/deserializing isn't equal for all datatypes regardless of byte size). We have to make sure when I turn this on, we never change the size or contents of the buffer.

I gotta check to make sure this is what we want to test.

The print alone doesn't tell me what the size should be, though, if I later come in and modify the benchmark. Ideally, this would have been expressed as an assert(data.lengthInBytes == expectation). But asserts are disabled here. Maybe just throw if it doesn't have the expected size?

I removed the print statement and instead added a comment that warns about ever editing that line of code. I don't think there is anything else we can do or assert.

dev/benchmarks/platform_channels_benchmarks/lib/main.dart

goderbauer · 2021-04-29T16:39:55Z

dev/benchmarks/platform_channels_benchmarks/lib/main.dart

+  );
+  printer.addResult(
+    description:
+        '  BasicMessageChannel/StandardMessageCodec/Flutter->Host/Large',


gaaclarke · 2021-04-29T20:23:35Z

There's a lot of boilerplate in this PR, I hope I reviewed all the right files...

Would probably be good if we found a way to not duplicate all this boilerplate for different benchmarks.

I was able to remove most of that boilerplate. The test is a bit more fragile now since I needed to keep some of android/ and ios/ to work. If the format of the template changes it could break these tests. It also adds a slight complication if people want to run the tests outside of devicelab for debugging purposes. I added a note in the README file about it.

dnfield

Mostly nits here. You're going to need to update dev/prod_builders.json to add a builder entry for this, and a corresponding PR will need to be opened in flutter/infra (see e.g. https://github.com/flutter/infra/pull/423) to make a builder for this actually run.

dnfield · 2021-05-03T22:25:04Z

dev/benchmarks/platform_channels_benchmarks/lib/main.dart

+  assert(false,
+      "Don't run benchmarks in checked mode! Use 'flutter run --release'.");


I'm pretty sure this was copied from elsewhere, but if we really want to check for release mode I think we want to do

Suggested change

assert(false,

"Don't run benchmarks in checked mode! Use 'flutter run --release'.");

if (!kReleaseMode)

throw Exception("Must be run in release mode! Use 'flutter run --release'.");

The assert will not be run in profile mode, and checked mode is an antiquated/deprecated notion at this point.

done (checked against kDebugMode)

Don't we want this check so that we make sure CI is running these in release mode?

These benchmarks just rely on stdout to get timing information, rather than the observatory.

No, there is a comment somewhere else that the tests have to be run in profile mode to work on iOS. Plus the only real problem case is Debug if people are running them locally.

dnfield · 2021-05-03T22:26:13Z

dev/benchmarks/platform_channels_benchmarks/lib/main.dart

+  final List<Object> largeBuffer = _makeTestBuffer(1000);
+  const StandardMessageCodec codec = StandardMessageCodec();
+  final ByteData data = codec.encodeMessage(largeBuffer);
+  print('Large buffer size: ${data.lengthInBytes}');


Can we somehow capture this in the benchmark metric? Like, maybe make it us/byte or something? Or just include the number of bytes processed somewhere in the metrics?

dnfield · 2021-05-03T22:27:36Z

dev/benchmarks/platform_channels_benchmarks/lib/main.dart

+}
+
+void main() {
+  runApp(const MyApp());


That's supposed to set it up though, right?

If not, can we just directly do what's needed to set up the default binary messenger?

dev/devicelab/lib/tasks/microbenchmarks.dart

dnfield

LGTM with nits

dev/prod_builders.json

...chmarks/android/app/src/main/kotlin/com/example/platform_channels_benchmarks/MainActivity.kt

goderbauer · 2021-05-04T16:39:11Z

dev/benchmarks/platform_channels_benchmarks/lib/main.dart

+  final List<Object> largeBuffer = _makeTestBuffer(1000);
+  const StandardMessageCodec codec = StandardMessageCodec();
+  final ByteData data = codec.encodeMessage(largeBuffer);
+  print('Large buffer size: ${data.lengthInBytes}');


I gotta check to make sure this is what we want to test.

The print alone doesn't tell me what the size should be, though, if I later come in and modify the benchmark. Ideally, this would have been expressed as an assert(data.lengthInBytes == expectation). But asserts are disabled here. Maybe just throw if it doesn't have the expected size?

goderbauer · 2021-05-04T16:41:02Z

dev/benchmarks/platform_channels_benchmarks/lib/main.dart

+}
+
+void main() {
+  runApp(const MyApp());


I think we should remove everything from the benchmark that's not required for it to make it clear what this is testing. Since this benchmark doesn't rely on any UI, why show it? If you really need to show UI, then just show a simple container.

dev/devicelab/lib/tasks/microbenchmarks.dart

goderbauer

LGTM

fluttergithubbot · 2021-05-04T19:04:04Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite Windows build_aar_module_test has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Windows build_tests_1_3 has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Windows build_tests_2_3 has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Windows build_tests_3_3 has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Windows framework_tests_libraries has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Windows framework_tests_misc has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Windows framework_tests_widgets has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Windows gradle_plugin_bundle_test has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Windows module_custom_host_app_name_test has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Windows module_test has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Windows tool_integration_tests_2_5 has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Windows tool_integration_tests_3_5 has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Windows tool_integration_tests_5_5 has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Windows web_tool_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

fluttergithubbot · 2021-05-04T21:39:20Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite Windows tool_tests_commands has failed. Please fix the issues identified (or deflake) before re-applying this label.

… to run the test now

keyonghan

Normally the json file change should be landed after builder config changes in flutter/infra. Please see #79399

keyonghan · 2021-05-07T17:44:36Z

dev/prod_builders.json

@@ -372,6 +372,12 @@
      "task_name": "linux_picture_cache_perf__e2e_summary",
      "flaky": false
    },
+    {
+      "name": "Linux platform channels benchmarks",


Shall the name be: Linux platform_channels_benchmarks?

keyonghan · 2021-05-07T17:45:33Z

dev/prod_builders.json

@@ -1152,6 +1158,12 @@
      "task_name": "mac_ios_platform_channel_sample_test_swift",
      "flaky": false
    },
+    {
+      "name": "Mac_ios platform channels benchmarks",


flutter-dashboard bot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Apr 28, 2021

google-cla bot added the cla: yes label Apr 28, 2021

gaaclarke mentioned this pull request Apr 28, 2021

add microbenchmark for platform channels #81348

Closed

8 tasks

gaaclarke force-pushed the platform-channels-benchmarks branch 9 times, most recently from 3b4c848 to 374976d Compare April 28, 2021 22:48

gaaclarke marked this pull request as ready for review April 28, 2021 23:29

This comment has been minimized.

Sign in to view

gaaclarke requested a review from goderbauer April 28, 2021 23:30

gaaclarke mentioned this pull request Apr 29, 2021

Deprecate dev/benchmarks/microbenchmarks #81394

Closed

goderbauer reviewed Apr 29, 2021

View reviewed changes

gaaclarke requested a review from goderbauer April 29, 2021 20:23

gaaclarke mentioned this pull request Apr 30, 2021

Moved PlatformMessage's to unique_ptrs flutter/engine#25860

Merged

9 tasks

dnfield reviewed May 3, 2021

View reviewed changes

dnfield approved these changes May 4, 2021

View reviewed changes

dnfield reviewed May 4, 2021

View reviewed changes

dev/prod_builders.json Show resolved Hide resolved

goderbauer requested changes May 4, 2021

View reviewed changes

gaaclarke requested a review from goderbauer May 4, 2021 17:30

goderbauer approved these changes May 4, 2021

View reviewed changes

gaaclarke added the waiting for tree to go green label May 4, 2021

fluttergithubbot removed the waiting for tree to go green label May 4, 2021

gaaclarke force-pushed the platform-channels-benchmarks branch from b8226a6 to 095c3a1 Compare May 4, 2021 20:12

gaaclarke added the waiting for tree to go green label May 4, 2021

fluttergithubbot removed the waiting for tree to go green label May 4, 2021

gaaclarke mentioned this pull request May 5, 2021

Added binary platform channels test #81874

Merged

8 tasks

gaaclarke added 16 commits May 7, 2021 09:11

Implemented performance tests for platform channels.

e789de6

added more missing copyright notices

3323fd1

ran upgrade on the pubspec

d0663f9

updatd to match new lint rules

468c10e

removed boilerplate code

31168d5

goderbauer's feedback

1abcbee

fixed print statement

78e20d6

ooops stray async

30699ac

added prod_builders

bb4b2c9

moved shared code into more appropriate positions

002d7d7

added missing license

f16f179

removed unnecessay ui code

8e38bdc

removed assert

2f5b746

added warning comment, removed print

c15cd17

moved the android tests to linux

d08e607

changed the number of messages to 2500 since it takes over 10 seconds…

8021eb2

… to run the test now

gaaclarke force-pushed the platform-channels-benchmarks branch from 095c3a1 to 8021eb2 Compare May 7, 2021 16:11

gaaclarke added the waiting for tree to go green label May 7, 2021

sorted imports

8ae9ea1

fluttergithubbot merged commit a5f57b9 into flutter:master May 7, 2021

keyonghan reviewed May 7, 2021

View reviewed changes

gaaclarke mentioned this pull request May 8, 2021

iOS platform channel benchmarks aren't running #82081

Closed

jmagman mentioned this pull request May 12, 2021

Mark linux_platform_channels_benchmarks not flaky #82384

Merged

		assert(false,
		"Don't run benchmarks in checked mode! Use 'flutter run --release'.");

Added performance benchmarks for platform channels #81414

Added performance benchmarks for platform channels #81414

Uh oh!

Conversation

gaaclarke commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

This comment has been minimized.

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Apr 29, 2021

Uh oh!

dnfield 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

Uh oh!

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

fluttergithubbot commented May 4, 2021

Uh oh!

gaaclarke commented Apr 28, 2021 •

edited

Loading