Shader warm up #27660

liyuqian · 2019-02-07T23:54:00Z

This patch adds a default shader warm up process which moves shader compilation from the animation time to the startup time. This also provides an extension for runApp so developers can customize the warm up process.

This should reduce our worst_frame_rasterizer_time_millis from ~100ms to ~20-30ms for both flutter_gallery and complex_layout benchmarks. Besides, this should also have a significant improvement on 90th and 99th percentile time (50%-100% speedup in some cases, but I haven't tested them thoroughly; I'll let our device lab collect the data afterwards).

The tradeoff the is the startup time (time to first frame). Our flutter run --profile --trace-startup seems to be a little noisy and I see about 100ms-200ms increase in that measurement for complex_layout and flutter_gallery. Note that this only happens on the first run after install or data wipe. Later the Skia persistent cache will remove the overhead.

This also adds a cubic_bezier benchmark to test the custom shader warm up process.

This should fix #813 (either by defaultShaderWarmUp, or a customShaderWarmUp).

chinmaygarde · 2019-02-08T00:15:17Z

By how much does this regress the time to first frame benchmarks?

liyuqian · 2019-02-08T00:56:54Z

@chinmaygarde : time to first frame increases about 100ms-200ms for complex_layout and flutter_gallery. Please see my updated PR description.

chinmaygarde

Thanks for discussing the tradeoffs and providing data. The approach looks good to me. I am sure we can do a better job of documenting how users can use this feature but that can be tackled in future commits. LGTM but someone more familiar with dart conventions should probably also take a look.

chinmaygarde · 2019-02-08T01:10:10Z

packages/flutter/lib/src/widgets/binding.dart

@@ -757,6 +757,10 @@ mixin WidgetsBinding on BindingBase, SchedulerBinding, GestureBinding, RendererB
 ///
 /// Initializes the binding using [WidgetsFlutterBinding] if necessary.
 ///
+/// Provide [customShaderWarmUp] if some specific Skia shaders need to be


I think this can be elaborated a bit to make it more useful. Something like, "if the application has scenes that require the compilation of complex shaders, it may cause jank in the middle of an animation or interaction. Painting that scene here tells Flutter to precompute and cache the results of expensive operations so they can be reused later. This should only happen during the very first run of the Flutter application and is typically very small. It is advised that you add tracing here so that such pauses are easier to identify on the timeline. The applications main thread will not be blocked when Flutter is doing this work so you should no expect ANR warnings".

Something like that.

Thanks! I've combined yours with my original documentation. I omit the tracing part because it's already added to _warmUpSkiaShaderCompilations which calls either [defaultShaderWarmUp] or [customShaderWarmUp].

CC @sfshaza2 for more API doc review.

To avoid dependency loop

jonahwilliams · 2019-02-08T18:38:29Z

packages/flutter/lib/src/foundation/shader_warm_up.dart

+/// to move common shader compilations from animation time to startup time.
+/// Alternatively, [customShaderWarmUp] can be provided to [runApp] to replace
+/// this default warm up function.
+ShaderWarmUp defaultShaderWarmUp = (Canvas canvas) {


You can define the function as void defaultShaderWarmUp(Canvas canvas) { and it will still be assignable to ShaderWarmUp

I intentionally wrote ShaderWarmUp here to clarify that the canvas is expected to have a size of 1000x1000. (Otherwise, I could delete ShaderWarmUp and just use the signature directly everywhere.)

I mean in terms of how the function is declared:

Suggested change

ShaderWarmUp defaultShaderWarmUp = (Canvas canvas) {

void defaultShaderWarmUp(Canvas canvas) {

I understood that. My intention is that when developers see the signature of defaultShaderWarmUp, they can discover a link to ShaderWarmUp and realize that we're doing work on a 1000x1000 canvas. Does that help?

If you leave a comment in the dartdoc [ShaderWarmup] they can follow that link as welll

Done. Thanks!

jonahwilliams · 2019-02-08T18:39:57Z

packages/flutter/lib/src/foundation/shader_warm_up.dart

+  ];
+
+  // Warm up path stroke and fill shaders.
+  for (Path path in paths) {


If we're trying to make this as fast as possible, index iteration is slightly faster than an iterator protocol

might want to benchmark this. I'm not sure how true that really is in practice.

goderbauer · 2019-02-13T22:58:32Z

@liyuqian Is this ready to land?

liyuqian · 2019-02-14T18:03:55Z

@goderbauer I guess so if the review from @jonahwilliams is a green light on the Dart side.

BTW, as I'm digging emails, I see that we're in a tree freeze due to P0 Google3/Fuchsia roll failure. Should I wait the tree to be reopened before merging this?

jonahwilliams · 2019-02-14T18:09:27Z

per the addition to runApp, I do not have a strong opinion on the API but others might

jonahwilliams

LGTM with nit

liyuqian · 2019-02-14T18:17:35Z

CC @Hixie for the review of API change to runApp.

Hixie · 2019-02-14T18:30:19Z

packages/flutter/lib/src/foundation/shader_warm_up.dart

@@ -0,0 +1,84 @@
+import 'dart:ui';


please see the README in this directory.

This probably belongs in the painting library, not foundation.

Moved to painting. Painting was my first choice but analyze won't allow me to do that since I used it in scheduler, which creates a dependency loop. Now I've removed all changes to scheduler and ShaderWarmUp is in painting again.

Hixie · 2019-02-14T19:00:42Z

packages/flutter/lib/src/scheduler/binding.dart

@@ -5,10 +5,11 @@
 import 'dart:async';
 import 'dart:collection';
 import 'dart:developer';
-import 'dart:ui' show AppLifecycleState;
+import 'dart:ui' show AppLifecycleState, Canvas, Image, Picture, PictureRecorder;


the scheduler shouldn't know about these things. They have nothing to do with scheduling.

Removed changes to scheduler.

Hixie · 2019-02-22T20:40:34Z

dev/benchmarks/macrobenchmarks/lib/common.dart

@@ -1 +1,2 @@
 const String kCullOpacityRouteName = '/cull_opacity';


missing license

Hixie · 2019-02-22T20:40:39Z

dev/benchmarks/macrobenchmarks/lib/main.dart

@@ -1,6 +1,7 @@
 import 'package:flutter/material.dart';


missing license

Hixie · 2019-02-22T20:41:25Z

dev/benchmarks/macrobenchmarks/test_driver/util.dart

@@ -0,0 +1,40 @@
+import 'dart:async';


missing license

Hixie · 2019-02-22T20:43:08Z

packages/flutter/lib/src/painting/shader_warm_up.dart

+  /// order of magnitude of most devices.
+  ///
+  /// A custom shader warm up can override this based on targeted devices.
+  ui.Size get size => const ui.Size(1024, 1024);


please use .0 on integral double literals

Hixie · 2019-02-22T20:44:50Z

cc @goderbauer

liyuqian · 2019-02-22T23:36:36Z

This patch adds a default shader warm up process which moves shader compilation from the animation time to the startup time. This also provides an extension for runApp so developers can customize the warm up process.

This should reduce our worst_frame_rasterizer_time_millis from ~100ms to ~20-30ms for both flutter_gallery and complex_layout benchmarks. Besides, this should also have a significant improvement on 90th and 99th percentile time (50%-100% speedup in some cases, but I haven't tested them thoroughly; I'll let our device lab collect the data afterwards).

The tradeoff the is the startup time (time to first frame). Our flutter run --profile --trace-startup seems to be a little noisy and I see about 100ms-200ms increase in that measurement for complex_layout and flutter_gallery. Note that this only happens on the first run after install or data wipe. Later the Skia persistent cache will remove the overhead.

This also adds a cubic_bezier benchmark to test the custom shader warm up process.

This should fix #813 (either by defaultShaderWarmUp, or a customShaderWarmUp).

This reverts commit a44f174. Reason: start_up tests become flaky. See flutter#28374 TBR: xster

This reverts commit a44f174. Reason: start_up tests become flaky. See #28374 TBR: xster Merge on red to fix the tree

This reverts commit adc8e15.

This reverts commit adc8e15. This should be safe to land once #28530 gets merged Merge on yellow doc test because the doc test is actually green.

ivnsch · 2019-03-18T23:06:00Z

I saw some back and forth with this PR in master, is now the final version merged?
Asking because I'm running the gallery demo from master on my iPhone 6 and it's still janky the first time. Up to date, with flutter run --release.

liyuqian · 2019-03-18T23:22:09Z

I assume that it's still janky when you reopen the app and run the animation for the first time? If so, I guess that it's caused by something else. Can you please run the gallery demo with flutter run --profile --trace-skia, save the trace from observatory timeline, and upload it here so we can take a deeper look?

ivnsch · 2019-03-19T07:38:54Z

The same happens after closing the app (where closing means really closing, not only sending to the background).

Attached the trace. This is of a new installation, where I opened "Studies". Then for each of the 4 items inside, opened it and pressed back. There was jank in all of them (the push animation almost jumps to its finished state, in some more than the others).

dart-timeline-2019-2-19.json.zip

msw333 · 2019-03-19T16:00:22Z

I can confirm that jittering on first startup still exists on iOS (just create any type of app with a list with 30 items with listview.builder and scroll it when you launch the app).

Hixie · 2019-03-19T17:46:27Z

I recommend that anyone running into slowness on startup file a bug with a small repro test case. There are probably many unrelated causes. This PR was to solve #813 which was specifically about the slowness in the Gallery.

ivnsch · 2019-03-19T17:59:13Z

@Hixie My comment was about the gallery (not sure if you're addressing me as well).

liyuqian · 2019-03-21T23:19:29Z

I think Hixie's comment is only for @msw333 . For that list with 30 items issue, it would helpful if @msw333 could create a new issue, attach the trace and a single-file Flutter app that can allow us to reproduce the issue.

For @i-schuetz 's issue, yes it's currently still expected to have a little jank right after the new installation. Your trace is consistent with my local experiment with a new installation. But once I close the app and reopen it on my iPhone6S, the jank would disappear if I open the same item. Can you please confirm that it's also the case in your iPhone6? If not, can you please attach the trace of the second first-run after the installation?

Hixie · 2019-03-23T00:21:27Z

We care about first-run performance too, very much in fact. :-)

msw333 · 2019-03-23T01:13:11Z

Ok ;-). Hope that helps. Issue is observable in every flutter app I was using. iPhone X, 12.1.4 (iOS).
Issue is more observable when using a list which is a little more complex.

How to reproduce:

Open a flutter app with a list on iOS, e.g. my code sample below.
Switch to a different app and scroll a list.
Switch back to the flutter app via iOS swipe gesture and directly scroll the list.
Same behavior is observable on first startup of the flutter app
(within profile mode you see how fps drop into the red)

Videos don't give it justice, but show the issue:
https://www.youtube.com/watch?v=oHKNKi08qw8
https://www.youtube.com/watch?v=1mdM_Uv574w

Code to reproduce (just use as main.dart). Again, I can reproduce it with any flutter app on iOS. Jittering issue increases within apps with higher complexity:
https://gist.github.com/msw333/dd0a06fb1fe8b4eb4ae627542202d43a

liyuqian · 2019-03-24T17:43:32Z

Thanks @msw333 ! That's very helpful. Let's further discuss this issue on #28113

ivnsch · 2019-03-26T08:36:24Z

Moved to #28113 (comment)

liyuqian added 6 commits February 6, 2019 12:09

[wip] Warnp Skia shaders during scheduleWarmUpFrame

92edcb0

Allow custom warm up

d1b7f82

Improve comments

36d642c

Log timeline and fix canvas bug

3ac40ad

Example app to visualize defaultShaderWarmUp

8dc36af

Add tests

60ae6db

liyuqian requested review from chinmaygarde and Hixie February 7, 2019 23:54

googlebot added the cla: yes label Feb 7, 2019

liyuqian changed the title ~~Shader warmup~~ Shader warm up Feb 7, 2019

Fix analyze

2b8565e

chinmaygarde approved these changes Feb 8, 2019

View reviewed changes

Move ShaderWarmUp to foundation

5047d9c

To avoid dependency loop

zoechi added framework flutter/packages/flutter repository. See also f: labels. c: performance Relates to speed or footprint issues (see "perf:" labels) labels Feb 8, 2019

jonahwilliams reviewed Feb 8, 2019

View reviewed changes

liyuqian added 2 commits February 8, 2019 12:00

No Colors in foundation

8b54e77

Improve doc and loop

a851875

liyuqian requested a review from sfshaza2 February 8, 2019 21:43

jonahwilliams approved these changes Feb 14, 2019

View reviewed changes

Simplify defaultShaderWarmUp signature

828350b

Hixie reviewed Feb 14, 2019

View reviewed changes

Hixie reviewed Feb 22, 2019

View reviewed changes

Adding missing licenses.

cea0143

liyuqian merged commit a44f174 into flutter:master Feb 22, 2019

This was referenced Feb 22, 2019

Check LICENSE in flutter/flutter #28368

Closed

start_up tests in our post-submit bots seem to become flaky #28374

Closed

liyuqian added a commit to liyuqian/flutter that referenced this pull request Feb 23, 2019

Revert "Shader warm up (flutter#27660)"

3c8cb21

This reverts commit a44f174. Reason: start_up tests become flaky. See flutter#28374 TBR: xster

liyuqian added a commit that referenced this pull request Feb 23, 2019

Revert "Shader warm up (#27660)" (#28376)

adc8e15

This reverts commit a44f174. Reason: start_up tests become flaky. See #28374 TBR: xster Merge on red to fix the tree

miguelcmedeiros mentioned this pull request Feb 25, 2019

Jittery performance on first run of animation/transitions due to shader compilations #813

Closed

liyuqian mentioned this pull request Feb 26, 2019

Fix type issue of the timelineEvents assignment #28530

Merged

liyuqian added a commit to liyuqian/flutter that referenced this pull request Feb 26, 2019

Reland "Shader warm up (flutter#27660)"

260a519

This reverts commit adc8e15.

liyuqian added a commit that referenced this pull request Feb 27, 2019

Reland "Shader warm up (#27660)" (#28537)

19a6a6f

This reverts commit adc8e15. This should be safe to land once #28530 gets merged Merge on yellow doc test because the doc test is actually green.

liyuqian mentioned this pull request Mar 15, 2019

complex_layout_scroll_perf__timeline_summary worst_frame_rasterizer_time_millis is twice slow (40ms => 90ms) #21393

Closed

liyuqian mentioned this pull request Mar 24, 2019

Jittering on Flutter Apps (iOS) when switched #28113

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 7, 2021

	ShaderWarmUp defaultShaderWarmUp = (Canvas canvas) {
	void defaultShaderWarmUp(Canvas canvas) {

		@@ -1 +1,2 @@
		const String kCullOpacityRouteName = '/cull_opacity';

Shader warm up #27660

Shader warm up #27660

Uh oh!

Conversation

liyuqian commented Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chinmaygarde commented Feb 8, 2019

Uh oh!

liyuqian commented Feb 8, 2019

Uh oh!

chinmaygarde 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!

goderbauer commented Feb 13, 2019

Uh oh!

liyuqian commented Feb 14, 2019

Uh oh!

jonahwilliams commented Feb 14, 2019

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

liyuqian commented Feb 14, 2019

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!

Hixie commented Feb 22, 2019

Uh oh!

liyuqian commented Feb 22, 2019

Uh oh!

ivnsch commented Mar 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liyuqian commented Mar 18, 2019

Uh oh!

liyuqian commented Feb 7, 2019 •

edited

Loading

ivnsch commented Mar 18, 2019 •

edited

Loading

ivnsch commented Mar 19, 2019 •

edited

Loading

msw333 commented Mar 23, 2019 •

edited

Loading

ivnsch commented Mar 26, 2019 •

edited

Loading