Add a new package: measure by liyuqian · Pull Request #31 · flutter/packages

liyuqian · 2019-08-30T00:00:32Z

For flutter/flutter#39439 and flutter/flutter#33899

dnfield

A few changes in the comments.

I would prefer not shipping the binary as a resource - we could have it as a pre-requisite instead, and just install it on the bots we need it on.

Alternatively, we could make it be a CIPD package and have this tool fetch it from CIPD if needed (although I'd still probably prefer we just make it a CIPD package and have the bot fetch it via CIPD before running this tool if needed).

dnfield · 2019-08-30T00:02:37Z

packages/measure/README.md

Is there some way we can avoid packaging a native binary with this?

Not that I know of... This is probably due to my lack of iOS/Xcode/instruments knowledge. I'm happy to remove this binary if someone can parse the trace without using this binary.

Just discussed with Dan offline. We can use CIPD to serve the binary instead of uploading it to git.

Is CIPD accessible for anybody? Or is this packages only meant for "internal" use?

We have both a public and an internal one. This should be fine for public. Public is at https://chrome-infra-packages.appspot.com/p/flutter, internal is https://chrome-infra-packages.appspot.com/p/flutter_internal

(public is readable by all)

dnfield · 2019-08-30T00:03:10Z

packages/measure/README.md

This language will become outdated and probably not get updated. Can we just say something like Sample output: and get rid of the whole Eventually, we'd like to hook this...?

dnfield · 2019-08-30T00:12:37Z

packages/measure/lib/commands/ioscpugpu/new.dart

+  void _checkDevice() {
+    _device = argResults[kOptionDevice];
+    if (_device == null) {
+      final ProcessResult result = Process.runSync('flutter', <String>['devices']);


GitHub lost some comments :(

I think we should not do this. Flutter tool output is not guarnateed to be stable, and an upstream change could easily break this unintentionally. I would recommend making the device argument be required, and callers could use flutter devices or whatever other method to call it (in the devicelab, we know the device ID we're supposed to target anyway).

I've already let the devicelab provide the deviceId in flutter/flutter#39439

However, I'd like my local use and test to be a little easier (see the last test in measure_test.dart).

I've now removed the flutter and instead used instruments to find the deviceId.

dnfield · 2019-08-30T00:13:33Z

packages/measure/lib/commands/base.dart

+      kOptionTraceUtility,
+      abbr: 'u',
+      help:
+        'path to TraceUtility binary (https://github.com/Qusic/TraceUtility)',


Please make this a full sentence, starting with a capital and ending with a period. E.g. Specifies the path to the TraceUtility binary... (...)..

dnfield · 2019-08-30T00:13:38Z

packages/measure/lib/commands/base.dart

+      help:
+        'the process name used for filtering the instruments CPU measurements',
+      defaultsTo: kDefaultProccessName,
+    );


goderbauer · 2019-08-30T17:47:26Z

packages/measure/README.md

@@ -0,0 +1,28 @@
+# Install


Add a section explaining what this packages does / what it is for?

goderbauer · 2019-08-30T17:49:13Z

packages/measure/README.md

Is CIPD accessible for anybody? Or is this packages only meant for "internal" use?

liyuqian

Ready for another round of review.

liyuqian · 2019-09-05T22:03:29Z

packages/measure/README.md

@@ -0,0 +1,28 @@
+# Install


liyuqian · 2019-09-05T22:03:34Z

packages/measure/README.md

dnfield · 2019-09-05T22:35:49Z

packages/measure/lib/commands/base.dart

      kOptionOutJson,
      abbr: 'o',
-      help: 'json file for the measure result.',
+      help: 'Specifies the json file for outputing result.',


for result output.

dnfield · 2019-09-05T22:37:41Z

Can you remove the binary files from this PR now?

liyuqian

Oops, forget to git rm -r resources. Now they should have been removed.

liyuqian · 2019-09-05T22:48:26Z

packages/measure/lib/commands/base.dart

      kOptionOutJson,
      abbr: 'o',
-      help: 'json file for the measure result.',
+      help: 'Specifies the json file for outputing result.',


liyuqian · 2019-09-09T17:22:28Z

@dnfield : ready to review again 😄

dnfield

LGTM!

liyuqian requested review from digiter, dnfield and goderbauer August 30, 2019 00:00

liyuqian force-pushed the measure branch 2 times, most recently from 4a3dfeb to 67fc35a Compare August 30, 2019 00:08

Add a new package: measure

d34d5ad

liyuqian force-pushed the measure branch from 67fc35a to d34d5ad Compare August 30, 2019 00:10

dnfield suggested changes Aug 30, 2019

View reviewed changes

dnfield reviewed Aug 30, 2019

View reviewed changes

liyuqian added 2 commits August 29, 2019 17:33

Use instruments to find deviceId

92e44b3

Improve helps

76b5189

goderbauer reviewed Aug 30, 2019

View reviewed changes

liyuqian commented Sep 5, 2019

View reviewed changes

Use CIPD to manage resources

d60482e

liyuqian force-pushed the measure branch from 4bf07f4 to d60482e Compare September 5, 2019 22:05

dnfield reviewed Sep 5, 2019

View reviewed changes

Format and other fixes

299363a

liyuqian commented Sep 5, 2019

View reviewed changes

Only test on mac-os

cb9736e

dnfield approved these changes Sep 9, 2019

View reviewed changes

liyuqian merged commit db15561 into flutter:master Sep 9, 2019

liyuqian mentioned this pull request Sep 9, 2019

Simple animation costs high CPU/GPU/Power usages (~20%) on iPhone 6 flutter/flutter#31865

Closed

liyuqian deleted the measure branch November 20, 2019 00:26

Conversation

liyuqian commented Aug 30, 2019

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liyuqian Aug 30, 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!

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!

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!

dnfield commented Sep 5, 2019

Uh oh!

liyuqian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liyuqian commented Sep 9, 2019

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

liyuqian Aug 30, 2019 •

edited

Loading