Added binary platform channels test by gaaclarke · Pull Request #81874 · flutter/flutter

gaaclarke · 2021-05-05T00:13:24Z

This depends on #81414

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.

flutter-dashboard · 2021-05-06T21:13:17Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

gaaclarke · 2021-05-06T21:15:02Z

@dnfield Please give this a review. I know it's a bit of a pain because of Github's UI, but since the tree is blocked I don't want to get a huge backlog of reviews, too.

It might help if you click on the commits after 095c3a1, I think all of the changes are in main.dart It just cleans up that file and adds 2 more tests.

gaaclarke · 2021-05-07T17:34:23Z

@dnfield It's been rebased now so it should be easy to review, I'll add @goderbauer too since I know you said your schedule might be weird today.

dnfield

Couple formatting nits. The warmup lap should be pretty simple.

dnfield · 2021-05-07T17:30:09Z

dev/benchmarks/platform_channels_benchmarks/lib/main.dart

nit: no new line here, or add a comma to the end of the string and put the closing paren on a new line.

I tried that and it didn't look any better to my eyes. Having a trailing comma for a parameter list is odd, it's not like an array or map literal. This is what the automatic formatter wants in this case.

It's what the style guide wants: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#if-you-have-a-newline-after-some-opening-punctuation-match-it-on-the-closing-punctuation

https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#prefer-a-maximum-line-length-of-80-characters

We don't use dartfmt in this repo, although it can come close if you specify a longer line length like 120 or something.

Done, imo If you have a newline after some opening punctuation, match it on the closing punctuation. doesn't make any sense. What is "it" and what should it be matched with? I read it about 20 times trying to understand how it applies to this case and I still don't get it but from the examples it seems applicable =P

dnfield · 2021-05-07T17:30:26Z

dev/benchmarks/platform_channels_benchmarks/lib/main.dart

dnfield · 2021-05-07T17:30:37Z

dev/benchmarks/platform_channels_benchmarks/lib/main.dart

dnfield · 2021-05-07T17:33:58Z

dev/benchmarks/platform_channels_benchmarks/lib/main.dart

+Future<double> _runBasicStandardSmall(
+    BasicMessageChannel<Object> basicStandard) async {
+  final Stopwatch watch = Stopwatch();
+  watch.start();


For this and the one below, we should have a warmup lap. Even though the Dart code is AOT in profile/release modes, on at least the Android side we'll be hitting JVM code.

Nice catch before these are turned on, done.

Did you add/commit the changes? I'm not seeing it yet.

And in case it's not clear, what I mean by warmup here is to actually use the message channel at least once before you start the stop watch.

Yep it's there. Each function is called with the parameter of count = 1 before it is actually measured.

The stopwatch happens for the warmup, but the result is thrown away

Ahhh ok, thanks I missed that.

dnfield

LGTM

dnfield · 2021-05-07T18:53:34Z

dev/benchmarks/platform_channels_benchmarks/lib/main.dart

+  const int numMessages = 2500;
+
  final BenchmarkResultPrinter printer = BenchmarkResultPrinter();
+  await _runBasicStandardSmall(basicStandard, 1);


nit: here or in the method, consider adding ac omment about this being a warmup run.

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

google-cla bot added the cla: yes label May 5, 2021

gaaclarke mentioned this pull request May 5, 2021

BasicMessageChannel's with BinaryCodec aren't sending responses on Android #81875

Closed

gaaclarke force-pushed the binary-platform-channels-test branch from 7008340 to 468f58f Compare May 5, 2021 17:26

gaaclarke marked this pull request as ready for review May 6, 2021 21:13

gaaclarke requested a review from dnfield May 6, 2021 21:13

gaaclarke force-pushed the binary-platform-channels-test branch from a73c7ac to c55a359 Compare May 7, 2021 16:35

gaaclarke added 2 commits May 7, 2021 10:32

added platform channel benchmark with the binary codec

4c11cbb

added 1MB test

8a83dd3

gaaclarke force-pushed the binary-platform-channels-test branch from c55a359 to 8a83dd3 Compare May 7, 2021 17:32

dnfield suggested changes May 7, 2021

View reviewed changes

gaaclarke requested review from dnfield and goderbauer May 7, 2021 17:34

gaaclarke added 2 commits May 7, 2021 10:55

added warmup

8a210d2

added trailing commas

18d7219

dnfield approved these changes May 7, 2021

View reviewed changes

added warmup comments, tweaked the names to match keyong's feedback

44af930

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

gaaclarke merged commit 8694e19 into flutter:master May 7, 2021

Conversation

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

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented May 6, 2021

Uh oh!

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

Uh oh!

Uh oh!

gaaclarke commented May 7, 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!

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!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gaaclarke commented May 5, 2021 •

edited

Loading

gaaclarke commented May 6, 2021 •

edited

Loading