Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Startup flutter faster (Only access globals.deviceManager if actually setting something) #111461

Conversation

jensjoha
Copy link
Contributor

@jensjoha jensjoha commented Sep 13, 2022

This PR makes flutter (as measured with flutter test directly in the flutter directory which has no test directory, thus just writing "there's no test directory) startup faster by caching git calls.

When starting flutter currently the user-specified "device id" is unconditionally set on the device manager.
There is nothing technically wrong with that, but when a device id has not been provided by the user, and we don't otherwise need the device manager it is created for no reason.
This wouldn't be too bad, if it wasn't because it could take quite a while to do so, but it can (because it eagerly searches for the android sdk).

This PR makes it conditional on the user having actually specified a device id.
A future PR could make the android sdk search lazy instead.

On my Linux machine - which does have an android sdk - this doesn't change much --- it removes something along the lines of 3-5 ms, but on my Windows machine - which does not have an android sdk - the change is quite significant, see below.

This PR stacks with #111392 and #111459.

Windows (Google issued with whatever security software --- and no android sdk)

Before This PR alone This PR stacked with #111392 and #111459
3.3373447 2.8303869 1.2849433
3.3226704 2.7771196 1.2759344
3.3039702 2.7602999 1.2668016
3.2984141 2.7172332 1.2741733
3.3244981 2.8190871 1.2679842
3.2896061 2.7742435 1.2899466
3.3063319 2.8068465 1.2569228
3.2715855 2.7997578 1.287999
3.3431836 2.7171742 1.2548849
3.2870784 2.7108063 1.2840665

I.e. this PR alone:

Difference at 95.0% confidence
        -0.537173 +/- 0.0330875
        -16.2363% +/- 1.00009%
        (Student's t, pooled s = 0.0352147)

And stacked with #111392 and #111459:

Difference at 95.0% confidence
        -2.0341 +/- 0.0174607
        -61.4817% +/- 0.527756%
        (Student's t, pooled s = 0.0185832)

A more detailed analysis is available internally at go/FlutterStartupTimeAnalysis (which also discussed future PRs).

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Sep 13, 2022
@flutter-dashboard
Copy link

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 (don't just cc him here, he won't see it! He's on Discord!).

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.

@jonahwilliams
Copy link
Member

This seems like a reasonable change, but is something that may accidentally regress if not tested in some way. Perhaps we could cover this with a CI benchmark that measures start up time of the flutter tool?

@jensjoha if you'd like I'd be more than happy to walk you though adding one. since you're doing so much work here and elsewhere to improve performance I think it would be great to track it

@jensjoha
Copy link
Contributor Author

I agree that a benchmark would be good.
I had originally wanted one for #107395 as well, but couldn't seem to find anything about how to add one, so if you can shed some light on that, that would be great!

@jonahwilliams
Copy link
Member

I think the best example to work from would be something like the analyzer benchmark:

Future<TaskResult> analyzerBenchmarkTask() async {
await inDirectory<void>(flutterDirectory, () async {
rmTree(_megaGalleryDirectory);
mkdirs(_megaGalleryDirectory);
await flutter('update-packages');
await dart(<String>['dev/tools/mega_gallery.dart', '--out=${_megaGalleryDirectory.path}']);
});
final Map<String, dynamic> data = <String, dynamic>{
...(await _run(_FlutterRepoBenchmark())).asMap('flutter_repo', 'batch'),
...(await _run(_FlutterRepoBenchmark(watch: true))).asMap('flutter_repo', 'watch'),
...(await _run(_MegaGalleryBenchmark())).asMap('mega_gallery', 'batch'),
...(await _run(_MegaGalleryBenchmark(watch: true))).asMap('mega_gallery', 'watch'),
};
return TaskResult.success(data, benchmarkScoreKeys: data.keys.toList());
}
class _BenchmarkResult {
const _BenchmarkResult(this.mean, this.min, this.max);
final double mean; // seconds
final double min; // seconds
final double max; // seconds
Map<String, dynamic> asMap(String benchmark, String mode) {
return <String, dynamic>{
'${benchmark}_$mode': mean,
'${benchmark}_${mode}_minimum': min,
'${benchmark}_${mode}_maximum': max,
};
}
}

In this we simply run some command and record the total time. In this case, maybe just flutter -h. So there are a few parts to this:

  1. Add a function structured like the analyzer task function above.
  2. Add 3 files in dev/devicelab/bin/tasks.dart, one for each host platform variant. You could name them something like flutter_tool_startup__windows, flutter_tool_startup__macos, et cetera. They can all have almost the same content and just wrap the call to the task function in a main.
  3. Add 3 new entries to the ci.yaml file https://github.com/flutter/flutter/blob/master/.ci.yaml that refers to the tasks you added above. I think Linux_android analyzer_benchmark is a good example to copy from. All of these should be marked bringup: true.
  4. Add entries to TESTOWNERS for each test with yourself + flutter/tool as an owner.

If you land this before any of these PRs, then we can observe the improvements too

jensjoha added a commit to jensjoha/flutter that referenced this pull request Sep 15, 2022
jensjoha added a commit that referenced this pull request Sep 16, 2022
Add flutter startup benchmark for Linux, Windows and MacOs.

Via guide on #111461 (comment)
@jonahwilliams
Copy link
Member

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants