Skip to content

Update shader warm-up for recent Skia changes#37955

Merged
liyuqian merged 4 commits intoflutter:masterfrom
liyuqian:update_gallery_shader_warm_up
Aug 15, 2019
Merged

Update shader warm-up for recent Skia changes#37955
liyuqian merged 4 commits intoflutter:masterfrom
liyuqian:update_gallery_shader_warm_up

Conversation

@liyuqian
Copy link
Copy Markdown
Contributor

@liyuqian liyuqian commented Aug 9, 2019

The update is copied from an update we made to a Google-internal
client: cl/260202900

The update will save 1 shader compilation.

This should help solve our regression:
#31203

More regressions on iOS might be introduced later by
flutter/engine#9813 (comment)

Unfortunately, we didn't rebase our benchmarks so such regressions
were not detected.

Hence to fully solve #31203,
we might need to revert some change in
flutter/engine#9813 to make iOS shader warm-up
happen on the GPU thread again.

The update is copied from an update we made to a Google-internal
client: cl/260202900

The update will save 1 shader compilation.

This should help solve our regression:
flutter#31203

More regressions on iOS might be introduced later by
flutter/engine#9813 (comment)

Unfortunately, we didn't rebase our benchmarks so such regressions
were not detected.

Hence to fully solve flutter#31203,
we might need to revert some change in
flutter/engine#9813 to make iOS shader warm-up
happen on the GPU thread again.
@fluttergithubbot fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Aug 9, 2019
@fluttergithubbot
Copy link
Copy Markdown
Contributor

It looks like this pull request may not have tests. Please make sure to add tests before merging. While there are exceptions to this rule, if this patch modifies code it is probably not an exception.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@liyuqian liyuqian added c: performance Relates to speed or footprint issues (see "perf:" labels) c: regression It was better in the past than it is now labels Aug 9, 2019
@liyuqian liyuqian force-pushed the update_gallery_shader_warm_up branch from c12d8a4 to 3eb967d Compare August 9, 2019 20:12
..restore();
canvas.translate(drawCallSpacing, 0.0);
}
canvas.translate(0.0, drawCallSpacing);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we add a test for this with a TestCanvas that just captures the method invocations and verifies they come out to what we expect? If you grep for TestCanvas you should find some examples

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

How about the added test?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! I'll defer to other reviewer(s) on final review for this.

@liyuqian
Copy link
Copy Markdown
Contributor Author

@liyuqian liyuqian requested a review from iskakaushik August 14, 2019 05:06
@liyuqian
Copy link
Copy Markdown
Contributor Author

@iskakaushik : since you're interested in shader warm-up, I wonder if you'd like to review this PR? For reference, go/flutter-compile-janks is an work-in-progress design doc that includes some backgrounds and solution comparisons. I'm also happy to give a brown bag if you're interested 😄

@liyuqian
Copy link
Copy Markdown
Contributor Author

Ping...

Copy link
Copy Markdown
Contributor

@iskakaushik iskakaushik left a comment

Choose a reason for hiding this comment

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

LGTM

@liyuqian liyuqian merged commit 77aa495 into flutter:master Aug 15, 2019
@liyuqian liyuqian deleted the update_gallery_shader_warm_up branch September 9, 2019 18:21
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

c: performance Relates to speed or footprint issues (see "perf:" labels) c: regression It was better in the past than it is now framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants