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
Perform no shader warm-up by default #87126
Conversation
TBH, I didn't know we were doing this by default! And that it was not doing anything useful (when it actually works) is not entirely unexpected as it requires constant upkeep, expertise in determining the triggers that generate the specific shader variants, context about the applications needs, and, is platform and rendering backend specific to boot. BTW, why provide the default at all? |
@@ -33,3 +33,141 @@ Future<void> main() async { | |||
ui.window.onBeginFrame = beginFrame; | |||
ui.window.scheduleFrame(); | |||
} | |||
|
|||
/// Default way of warming up Skia shader compilations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this just be deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I'll just remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to just delete the whole example. Filed #87132 to track just removing this API entirely
Image tracing expectations had to be updated because a counter that used to be 1 is now 0 - due to no shader warmup creating an image. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
So far it looks like this may be saving somewhere between 10-30ms on time to first frame. Will have to see if that's just noise in the benchmark or not. |
This also resulted in a big drop for the iOS time to first frame benchmarks. https://flutter-flutter-perf.skia.org/e/?begin=1620937746&end=1627447444&queries=sub_result%3DtimeToFirstFrameRasterizedMicros&requestType=0&xbaroffset=25119 |
It's also caused a big regression in the average frame time for one benchmark: https://flutter-flutter-perf.skia.org/e/?begin=1627417144&end=1627480450&keys=X92fe9c017ec15878ab0ff811545cb492&num_commits=50&request_type=1&xbaroffset=25205 |
Fixes #87085
Before flutter/engine#27629, on Android, the shader warm-up routine did not actually produce any shaders (!). This is because the backend ended up going down a raster path that doesn't use GPU shaders. It is completely wasted time.
Now that a GPU baacked surface is available in the background, it does produce shaders - however, those shaders do not seem to be significantly improving benchmarks, and are regressing startup time and memory usage.
We can consider re-applying a default routine if we can find one that actually improves metrics. In the mean time, we should remove this.
The internal customer this logic was written for has their own routine(s) that will not be affected by this change.