Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

fzyzcjy
Copy link
Contributor

@fzyzcjy fzyzcjy commented Oct 21, 2022

  1. I will finish code details, refine code, add tests, make tests pass, etc, after a code review that thinks the rough idea is acceptable. It is because, from my past experience, reviews may request changing a lot. If the general idea is to be changed, all detailed implementation efforts are wasted :)
  2. The PR has an already-working counterpart, and it produces ~60FPS smooth experimental results. The benchmark results and detailed analysis is in chapter https://cjycode.com/flutter_smooth/benchmark/. All the source code is in https://github.com/fzyzcjy/engine/tree/flutter-smooth and https://github.com/fzyzcjy/flutter/tree/flutter-smooth.
  3. Possibly useful as a context to this PR, there is a whole chapter discussing the internals - how flutter_smooth is implemented. (Link: https://cjycode.com/flutter_smooth/design/)

This fixes the jank happened in classical Flutter, even without the existence of flutter_smooth

I will add tests and refine code etc after some code review - since review may request changing the code :)

This works pretty well in flutter_smooth, see https://github.com/fzyzcjy/engine/blob/flutter-smooth/shell/common/animator.cc for full code.

During experiments, I observe a phenomenon: Even when the UI thread finishes everything before the deadline (vsync) a few milliseconds, the next frame is scheduled one vsync later, causing one jank. For example, UI thread may run from 0-15ms, but the next frame starts from 33.33ms instead of the correct 16.67ms.

An example screenshot can be seen at the end of this proposal. I added a timeline event, Animator::AwaitVSync, so we can clearly see when vsync await is called. (This screenshot has roughly 3ms space; but more frequently, I see this bug when there is about 0.5-2ms space.)

Therefore, this PR tries to fix this problem. The main idea is that, when detecting we are very near the next vsync, we do not wait at all, but instead directly start the next frame.

image

zoom in:

image

further zoom in:

image

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests. -- see above
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard
Copy link

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.

@dnfield
Copy link
Contributor

dnfield commented Oct 27, 2022

This seems very similar to #29276

Similar concerns as to that one: we need some motivating benchmarks that clearly show what benefits would be gained here, and we need to see that changes to this won't negatively impact existing benchmarks/users.

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Oct 27, 2022

Thanks for pointing out the related PR.

Similar concerns as to that one: we need some motivating benchmarks that clearly show what benefits would be gained here, and we need to see that changes to this won't negatively impact existing benchmarks/users.

Sure. I will try to make one.

we need to see that changes to this won't negatively impact existing benchmarks/users.

May I know how to do this? does not see benchmark data on CI IMHO

@dnfield
Copy link
Contributor

dnfield commented Oct 27, 2022

flutter-flutter-perf.skia.org and flutter-engine-perf.skia.org has benchmark data

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Oct 27, 2022

@dnfield Thanks, may I know some doc, or how to see perf benchmark of a PR? Click "help" gives http://go/perf-user-doc which cannot be opened (google internal only?)

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Nov 5, 2022

@dnfield

(content of this comment is moved to: #37341)

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Nov 5, 2022

(content is also moved to #37341)

@dnfield and we need to see that changes to this won't negatively impact existing benchmarks/users

Could you please give some hints? I guess I have no permission to execute skia perf as it seems only run on master or other flutter branches. Thus, maybe I can do nothing except for creating a PR and see skia perf after it is merged...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants