Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
jensjoha merged 1 commit into from
Sep 19, 2022
Merged

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

jensjoha merged 1 commit into from
Sep 19, 2022

Conversation

@jensjoha
Copy link
Contributor

@jensjoha jensjoha commented Sep 13, 2022 •
edited
Loading

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.

Currently when running flutter for the first time or after being on a new git hash, it is compiled to a kernel/dill file. Subsequent calls then just run this compiled file, and thus avoids having to compile flutter_tools again and again. This is good.
We also have the option to compile to to an app-jit snapshot though, which is better in the sense that it loads faster.

This PR does exactly that.
Note that this stacks with #111392.

The startup changes as measured on my machines is this:

Linux

Before This PR alone This PR stacked with #111392
0.583 0.307 0.158
0.583 0.287 0.16
0.574 0.296 0.171
0.593 0.308 0.164
0.589 0.297 0.177
0.618 0.305 0.164
0.579 0.301 0.164
0.59 0.295 0.167
0.604 0.301 0.16
0.572 0.295 0.161

I.e. this PR alone:

Difference at 95.0% confidence
        -0.2893 +/- 0.0102548
        -49.1589% +/- 1.74253%
        (Student's t, pooled s = 0.0109141)

And stacked with #111392:

Difference at 95.0% confidence
        -0.4239 +/- 0.0100685
        -72.0306% +/- 1.71087%
        (Student's t, pooled s = 0.0107158)

Windows (Google issued with whatever security software)

Before This PR alone This PR stacked with #111392
3.2045302 2.7633213 1.7806395
3.2686755 2.7909508 1.8164078
3.2457845 2.7613864 1.7898207
3.1958543 2.8141012 1.8039282
3.1954143 2.7816646 1.7866668
3.2369129 2.7448754 1.8005397
3.2849415 2.7315453 1.8290792
3.2987666 2.7639699 1.7882604
3.2835086 2.7211788 1.8032152
3.2308832 2.7489684 1.8420157

I.e. this PR alone:

Difference at 95.0% confidence
        -0.482331 +/- 0.0315589
        -14.866% +/- 0.972681%
        (Student's t, pooled s = 0.0335877)

And stacked with #111392:

Difference at 95.0% confidence
        -1.44047 +/- 0.0287394
        -44.3969% +/- 0.885781%
        (Student's t, pooled s = 0.030587)

The cost is the compilation taking ~7% longer (less than a second longer).
This seems like a good trade-off to me.

We can technically make it load even faster by using an aot-snapshot, but then the compilation time will increase from something like 10 seconds to something like 30 seconds, and I'm not sure that's a good trade-off.

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

@flutter-dashboard
Copy link

flutter-dashboard bot commented Sep 13, 2022

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.

This was referenced Sep 13, 2022
Merged
Merged
@goderbauer goderbauer added the tool Affects the "flutter" command-line tool. See also t: labels. label Sep 13, 2022
jonahwilliams
jonahwilliams reviewed Sep 15, 2022
View reviewed changes
Copy link
Contributor

@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.

I think I had tried this a few years ago, but I found that the increased compilation time was really quite noticeable. Has performance of app-jit generation improved in recent years?

I definitely don't mind trying this out though

@christopherfujino
Copy link
Contributor

christopherfujino commented Sep 15, 2022

I think I had tried this a few years ago, but I found that the increased compilation time was really quite noticeable. Has performance of app-jit generation improved in recent years?

I definitely don't mind trying this out though

Ahh ok, I remember you tried this but I forgot why it got reverted. Tool compilation should only be an issue either during flutter upgrade, or for the first time after a user checks out a new commit. Is this not reasonable?

@christopherfujino
Copy link
Contributor

christopherfujino commented Sep 15, 2022

Maybe the answer is we should also benchmark tool compilation?

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Sep 15, 2022

I don't really think that makes sense to track, unless we want to also track the time it takes to download all of the binaries. It only needs to be tolerable for folks that regularly contribute to the tool

@christopherfujino
Copy link
Contributor

christopherfujino commented Sep 15, 2022

I don't really think that makes sense to track, unless we want to also track the time it takes to download all of the binaries. It only needs to be tolerable for folks that regularly contribute to the tool

I didn't mean adding a skiaperf benchmark, but at least compiling it both ways n times and documenting on this PR

@jensjoha
Copy link
Contributor Author

jensjoha commented Sep 16, 2022 •
edited
Loading

I did that (on my Linux machine):

Kernel and app-jit is done basically as-is (i.e. --snapshot= and --snapshot-kind= flags to dart), whereas exe is done via dart compile exe.

Runtimes for snapshotting as measured via time.

  kernel app-jit exe
  10.035 10.44 32.967
  9.622 10.302 33.132
  9.435 10.253 33.231
  9.462 10.287 32.831
  9.49 10.186 32.535
       
Average 9.6088 10.2936 32.9392
  100.00% 107.13% 342.80%

Which is the data behind my claim of

The cost is the compilation taking ~7% longer (less than a second longer).

This is also why I'm not suggesting compiling to exe (or aot-snapshot) even though that is faster still (in regards to startup at least).

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Sep 16, 2022

I think the time increase is OK with me. WDUT @christopherfujino ?

@christopherfujino
Copy link
Contributor

christopherfujino commented Sep 16, 2022

Awesome, this looks great to me!

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Sep 16, 2022

Lets wait until we get the startup benchmarks running: #111748

jonahwilliams
jonahwilliams approved these changes Sep 16, 2022
View reviewed changes
Copy link
Contributor

@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

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Sep 16, 2022

Benchmarks are running: https://flutter-flutter-perf.skia.org/e/?queries=test%3Dflutter_tool_startup__linux

@jensjoha jensjoha merged commit 0abb98d into flutter:master Sep 19, 2022
@jensjoha jensjoha mentioned this pull request Sep 19, 2022
Merged
This was referenced Sep 19, 2022
Merged
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 19, 2022
@engine-flutter-autoroll
0abb98d Use app-jit snapshot for flutter tools (flutter/flutter#111459)
8608e96
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 19, 2022
@engine-flutter-autoroll
0abb98d Use app-jit snapshot for flutter tools (flutter/flutter#111459)
706b3bf
@zanderso
Copy link
Member

zanderso commented Sep 29, 2022

I think I had tried this a few years ago, but I found that the increased compilation time was really quite noticeable. Has performance of app-jit generation improved in recent years?

Previously: #30102. It was reverted due to a crash in the VM, and it looks like we didn't follow up =(. Presumably in the intervening years, the crasher has been fixed.

@jensjoha
Copy link
Contributor Author

jensjoha commented Sep 30, 2022

No. This failed to. I fixed it in #111879.

@jensjoha
Copy link
Contributor Author

jensjoha commented Sep 30, 2022

Oh, okay, that particular error I didn't see. Lets hope that was fixed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@christopherfujino christopherfujino Awaiting requested review from christopherfujino

1 more reviewer

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jensjoha @christopherfujino @jonahwilliams @zanderso @goderbauer