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
Call AwaitVSync directly when Animator.RequestFrame is calling #29276
Conversation
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. 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. |
Do you have any results from running this on any of the devicelab tests? You can run those with a locally built engine in the framework tree by doing |
I tested it with a high-end phone (HUAWEI P40 Pro) and a low-end phone (Vivo Y67), and the specific results are as follows: At the same time, I also recorded the time from
low-end device(VIVO Y67) with patch
high-end device(HUAWEI P40 Pro) without patch
high-end device(HUAWEI P40 Pro) with patch
Preliminary conclusion:Almost no effect on frame build time Advantage:
Disadvantage:
|
It looks like frame build times got slightly better over all, but with an average increase in the worst times. Raster times got worse across the board. |
This is very strange, this patch should not affect raster times in theory. |
Hi, @dnfield |
I will test it again tomorrow |
@dnfield At the same time I drew some pictures to illustrate the problem |
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. 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. |
b1a8771
to
2962099
Compare
From our call in person on December 09, we were seeking a benchmark you could share with us so we ensure we don't regress here. |
Gentle ping on the benchmark. |
@chinmaygarde |
Hi, did you have further experiments on this? Related: #36911 |
Sorry, I don't have a benchmark to support it. |
@ColdPaleLight Thank you all the same |
fixes flutter/flutter#92258
Currently this new feature is only support on Android and iOS,I have tried to enable this feature on all platforms, but the unit test AnimatorDoesNotNotifyIdleBeforeRender will fail due to timeout.
Pre-launch Checklist
writing and running engine tests.
///
).