Skip to content
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

Shell::OnPlatformViewCreated should not serialize on the UI thread #91711

Closed
dnfield opened this issue Oct 12, 2021 · 3 comments · Fixed by flutter/engine#29145
Closed

Shell::OnPlatformViewCreated should not serialize on the UI thread #91711

dnfield opened this issue Oct 12, 2021 · 3 comments · Fixed by flutter/engine#29145
Assignees
Labels
c: performance Relates to speed or footprint issues (see "perf:" labels) customer: money (g3) engine flutter/engine repository. See also e: labels. P1 High-priority issues at the top of the work list perf: startup Performance issues related to app startup time

Comments

@dnfield
Copy link
Contributor

dnfield commented Oct 12, 2021

This is a snip from a trace during Shell::OnCreatePlatformView. The highlighted section is where the call is running on the platform thread, and the picture is showing what's going on on the UI thread:

image

We should not be blocking progress on the platform thread while the UI thread is doing this work, and although the comment here says that this is important for some platforms, I'm not convinced that is true.

@chinmaygarde pointed out that the ordering is important in the OnPlatformViewDestroyed route, which I agree with and am less concerned about anyway. A couple concerns that he raised:

  • Will this interrupt ordering of OnPlatformViewDestroyed? No, because a platform task cannot get scheduled before we've enqueued the raster/UI work, and the raster/UI work it enqueues will be blocked by the tasks we've enqueued.
  • Will the logic to check whether the raster/platform thread have been merged still be valid? Yes, because the UI thread will not get a chance to create a platform view, which would require time on the platform runner, before we use that variable.
@dnfield dnfield added engine flutter/engine repository. See also e: labels. c: performance Relates to speed or footprint issues (see "perf:" labels) customer: money (g3) P1 High-priority issues at the top of the work list perf: startup Performance issues related to app startup time labels Oct 12, 2021
@dnfield dnfield self-assigned this Oct 12, 2021
@dnfield
Copy link
Contributor Author

dnfield commented Oct 12, 2021

/cc @gaaclarke who may have ideas about what this could break on iOS. /cc @blasten who probably has some more paged in for Android.

@dnfield
Copy link
Contributor Author

dnfield commented Oct 12, 2021

From chat, a test case Chinmay mentioned we should have for this:

In shell_unittests, create a shell that calls OnPlatformViewCreated, invoke main that calls back into native code to wait for a latch. This will block the UI thread. Then call PlatformViewDestroyed and release the latch. In dart code, the add a platform view and try to render a frame with it. See what happens?

@github-actions
Copy link

github-actions bot commented Nov 1, 2021

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 1, 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) customer: money (g3) engine flutter/engine repository. See also e: labels. P1 High-priority issues at the top of the work list perf: startup Performance issues related to app startup time
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant