Startup `flutter` faster (cache git calls) #111392

jensjoha · 2022-09-12T13:52:55Z

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 a number of git calls are made, which is quite expensive. This PR introduces caching of these calls.
Caching is deleted when also creating a new snapshot (i.e. it's guarded by the same mechanism, which is the git hash).

This does mean that these things can change:

Two branches on the same hash will have an undetected stale cache (in that it will think it's on one of the branch when it could be on another). I'll say this is very unlikely from having any real effect. Logically we might report crashes or whatever as if on the "main" branch when we're really on our own not-yet-committed-to copy of main, but logically this already happens when we haven't created a branch yet but have made changes to the code.
If official tags are created for old versions where the user cached the old answer (e.g. no tags, user uses flutter and caches that, a tag is then created, the user fetches (but stays on the old hash) --- git would now know about the new tag, but caching isn't updated. Again I'll say this doesn't matter and any crash reports or whatever would have to handle the case anyway because not everyone has necessarily fetched.

To me, neither of these seems like big deals though.
The startup changes as measured on my machines is this:

Linux

Before on master	Before not on master	With PR
0.614	0.606	0.484
0.655	0.602	0.482
0.69	0.605	0.489
0.669	0.595	0.474
0.647	0.598	0.493
0.672	0.584	0.483
0.638	0.602	0.483
0.67	0.614	0.489
0.67	0.593	0.48
0.677	0.609	0.489

While not important for this PR I'll note that it's faster to startup flutter when not on a branch called master:

Difference at 95.0% confidence
        -0.0594 +/- 0.0157866
        -8.99727% +/- 2.39118%
        (Student's t, pooled s = 0.0168015)

This PR improves things (compared to now on branch called master):

Difference at 95.0% confidence
        -0.1756 +/- 0.0151538
        -26.598% +/- 2.29533%
        (Student's t, pooled s = 0.016128)

This PR improves things (compared to now on branch not called master):

Difference at 95.0% confidence
        -0.1162 +/- 0.00681882
        -19.3409% +/- 1.13496%
        (Student's t, pooled s = 0.00725718)

I.e. it's somewhere from 20 to 25% faster, cutting off something like 110 to 175 ms.

Windows (Google issued with whatever security software)

Before on master	Before not on master	Now
3.2498843	3.2052244	2.323404
3.2471592	3.2368394	2.3255623
3.2630213	3.2279151	2.3146872
3.2694762	3.2593181	2.2819018
3.2471021	3.2587005	2.3097854
3.2608728	3.2641268	2.3177364
3.2796176	3.2531503	2.2960537
3.2498695	3.2752845	2.3057254
3.2417869	3.2853195	2.3059499
3.2782214	3.2681917	2.3037698

This PR improves things:

Difference at 95.0% confidence
        -0.944949 +/- 0.0180964
        -29.0449% +/- 0.556228%
        (Student's t, pooled s = 0.0192597)

I.e. it's almost 30% faster, cutting off almost a full second.

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

jensjoha · 2022-09-20T06:54:26Z

Any input on this one?

jonahwilliams · 2022-09-20T15:55:53Z

For the actual caching behavior you've outlined, I would defer to our resident git authority @christopherfujino .

I would say that in general I'm okay with trying this out because we have a lot of runway before our next release, but in general it does strike me as the change most likely to introduce a bug.

packages/flutter_tools/lib/src/version.dart

packages/flutter_tools/test/commands.shard/hermetic/upgrade_test.dart

jensjoha · 2022-09-21T07:37:42Z

Thanks for the feedback @jonahwilliams. It should all be incorporated now.
And a friendly ping for @christopherfujino =)

jonahwilliams

LGTM if LG to @christopherfujino

christopherfujino

I'm sorry I didn't review this one earlier. I'm not sure this is the right change to make, although I'm sure we can improve this. I'm pretty sure if we're on a release branch, we shouldn't need to ever fetch tags.

However, if we ARE on master, I don't think we want to cache this call, as there could have been a newer tag added after we checked to a commit that is an ancestor of our current HEAD, and we would want the Flutter version to report the commit as being more recent than that.

packages/flutter_tools/lib/src/version.dart

jensjoha · 2022-09-23T07:31:54Z

However, if we ARE on master, I don't think we want to cache this call, as there could have been a newer tag added after we checked to a commit that is an ancestor of our current HEAD, and we would want the Flutter version to report the commit as being more recent than that.

I don't understand this part. Either I don't understand what you're saying, or I don't understand why it should be the case (the way I'm reading it we should suddenly report an old version as being newer?).

Also it's not clear to me which command you think shouldn't be cached.
The cached ones are these:

git -c log.showSignature=false log -n 1 --pretty=format:%H. This command does not seem to fetch anything from the network (having [url "[email protected]:"]\n insteadof = https://github.com/ in my .gitconfig asks for my password for stuff like fetching, and this command does not ask for my password so I assume it doesn't access the network at all).
git rev-parse --abbrev-ref --symbolic @{upstream}. This command does not seem to fetch anything from the network.
git ls-remote --get-url origin. This command does not seem to fetch anything from the network.
git -c log.showSignature=false log HEAD -n 1 --pretty=format:%ad --date=iso. This command does not seem to fetch anything from the network.
git rev-parse --abbrev-ref HEAD. This command does not seem to fetch anything from the network.
git tag --points-at hash (e.g. git tag --points-at cbfb8fa2b2fe625db03535b313cd3f5957680ac1). This command does not seem to fetch anything from the network.
git describe --match *.*.* --long --tags hash (e.g. git describe --match *.*.* --long --tags cbfb8fa2b2fe625db03535b313cd3f5957680ac1). This command does not seem to fetch anything from the network.

So none of the cached commands access the network. So the only way the answers can become "externally out-of-date" is by other commands accessing the network:

git -c log.showSignature=false log -n 1 --pretty=format:%ar is not cached because it gives a relative time (e.g. 2 hours ago) but does not access the network.
git fetch https://github.com/flutter/flutter.git --tags -f. This command accesses the internet --- specifically to fetch tags. We should probably delete the cache after this call, so I've added that.
The user could manually run git fetch or similar. I don't think we can really guard against that. We could add a time-guard like in _getLatestAvailableFlutterDate (or possibly just delete the cache when actually getting the latest flutter date anyway).

christopherfujino · 2022-09-29T20:32:48Z

However, if we ARE on master, I don't think we want to cache this call, as there could have been a newer tag added after we checked to a commit that is an ancestor of our current HEAD, and we would want the Flutter version to report the commit as being more recent than that.

I don't understand this part. Either I don't understand what you're saying, or I don't understand why it should be the case (the way I'm reading it we should suddenly report an old version as being newer?).

Consider that you check out the master branch at commit abcd, which is 10 commits past the last tagged release, version 1.0.0. The output of git describe --match *.*.* --long --tags abcd would be something like 1.0.0~10.

Now consider we tag a commit 5 commits past version 1 and 5 commits before commit abcd as version 2.0.0. Now, the result of git describe --match *.*.* --long --tags abcd would be 2.0.0~5.

jensjoha · 2022-09-30T08:19:17Z

However, if we ARE on master, I don't think we want to cache this call, as there could have been a newer tag added after we checked to a commit that is an ancestor of our current HEAD, and we would want the Flutter version to report the commit as being more recent than that.

I don't understand this part. Either I don't understand what you're saying, or I don't understand why it should be the case (the way I'm reading it we should suddenly report an old version as being newer?).

Consider that you check out the master branch at commit abcd, which is 10 commits past the last tagged release, version 1.0.0. The output of git describe --match *.*.* --long --tags abcd would be something like 1.0.0~10.

Now consider we tag a commit 5 commits past version 1 and 5 commits before commit abcd as version 2.0.0. Now, the result of git describe --match *.*.* --long --tags abcd would be 2.0.0~5.

This would require the user to fetch new tags though. Meaning there will always be a window in which they see it as 1.0.0~10 even after it has been tagged, and thus the scenario already happens. I've inserted an invalidation in fetchRemoteFrameworkCommitDate() now (which is what's called from _getLatestAvailableFlutterDate if it's been too long since the last check) meaning that such cases should always be noticed within ~3 days.
In - I'm guessing - all likely scenarios (i.e. when the user doesn't run git fetch manually) this makes the tag description give the "wrong" result at exactly the same times as before.

jensjoha · 2022-10-03T08:53:43Z

ping.

christopherfujino

You know, I'm sorry to do this, but after having gone through this PR, I think we should actually just refactor this version reporting code to read from a version file checked in to the repo (tracked #112833). We will likely need to do this for SLSA compliance anyway.

jensjoha · 2022-10-04T06:07:26Z

In a private email I've been told that the above means that the PR has been rejected so I'm closing this PR.

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

jensjoha changed the title ~~Startup flutter faster~~ Startup flutter faster (cache git calls) Sep 12, 2022

jensjoha requested review from christopherfujino and jonahwilliams September 13, 2022 08:28

This was referenced Sep 13, 2022

Startup flutter faster (use app-jit snapshot) #111459

Merged

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

Merged

Startup flutter faster (faster wrapper script on Windows) #111465

Merged

jonahwilliams reviewed Sep 20, 2022

View reviewed changes