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.

Sign up for GitHub

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

Jump to bottom

[Impeller] Investigate removal of waitUntilScheduled in Metal backend. #120399

Closed
jonahwilliams opened this issue Feb 9, 2023 · 10 comments · Fixed by flutter/engine#41501
Closed

[Impeller] Investigate removal of waitUntilScheduled in Metal backend. #120399

jonahwilliams opened this issue Feb 9, 2023 · 10 comments · Fixed by flutter/engine#41501
Assignees
@bdero
Labels
e: impeller Impeller rendering backend issues and features requests engine flutter/engine repository. See also e: labels. P2 Important issues not at the top of the work list

Comments

@jonahwilliams
Copy link
Member

jonahwilliams commented Feb 9, 2023 •
edited by chinmaygarde
Loading

This is necessary today because Impeller doesn't do resource hazard tracking across render passes. This introduces bubbles in execution on the GPU which wastes available concurrency.
@jonahwilliams jonahwilliams added engine flutter/engine repository. See also e: labels. P3 Issues that are less important to the Flutter project e: impeller Impeller rendering backend issues and features requests labels Feb 9, 2023
@bdero bdero mentioned this issue Feb 9, 2023
Open
@chinmaygarde chinmaygarde changed the title [impeller] Investigate removal of waitUntilScheduled in metal backend [Impeller] Investigate removal of waitUntilScheduled in Metal backend. Feb 11, 2023
@jonahwilliams jonahwilliams mentioned this issue Feb 26, 2023
Closed
@jonahwilliams jonahwilliams mentioned this issue Mar 10, 2023
Closed
@jonahwilliams
Copy link
Member Author

I added some additional tracing so that we account for the waitUntiLScheduled in the SubmitCommands method. It appears that the vast majority of the time is spent just waiting.

image

@jonahwilliams
Copy link
Member Author

In the wonderous app, removing waitUntilScheduled reduces average raster time from 7-8ms down to 1-2ms.

@chinmaygarde
Copy link
Member

Did you attach the wrong screenshot? That is showing the wait for the next drawable. That is the backpressure mechanism (the GPU telling us we are running ahead of its ability to render the stuff we are telling it to) and this issue isn't describing getting rid of it.

I got rid of the need for this wait as I am wiring up the Vulkan backend and have some notes on how we can do the same for Metal. We can discuss it in the weekly or an OOB sync perhaps?

@jonahwilliams
Copy link
Member Author

No, take a look at the command buffer upload part. It's all waitUntilScheduled

@jonahwilliams
Copy link
Member Author

Weekly sounds fine fyi, just doing some investigating

@jonahwilliams
Copy link
Member Author

This is what I added for tracing, I believe it should be accurate.

@@ -164,6 +168,7 @@ bool CommandBufferMTL::OnSubmitCommands(CompletionCallback callback) {
   }
 
   [buffer_ commit];
+  TRACE_EVENT0("impeller", "CommandBufferMTL::waitUntilScheduled");
   [buffer_ waitUntilScheduled];
   buffer_ = nil;
   return true;

@chinmaygarde
Copy link
Member

Note for myself to try to assimilate the ordering guarantees in Metal.

@jonahwilliams jonahwilliams mentioned this issue Mar 17, 2023
Closed
8 tasks
@chinmaygarde
Copy link
Member

Fixed by @bdero in flutter/engine#40781. We were on the wrong lines of investigation w.r.t hazard tracking.

@chinmaygarde
Copy link
Member

The fix was reverted in flutter/engine#40895.

@chinmaygarde chinmaygarde reopened this Apr 3, 2023
@chinmaygarde chinmaygarde added P2 Important issues not at the top of the work list and removed P3 Issues that are less important to the Flutter project labels Apr 17, 2023
@bdero bdero mentioned this issue Apr 26, 2023
Merged
bdero added a commit to flutter/engine that referenced this issue Apr 26, 2023
@bdero
[Impeller] iOS/macOS: Only wait for command scheduling prior to prese…
86118e5 
…nt (redux) (#41501)

Reverts #40895.
Resolves flutter/flutter#120399 (again).

A bunch of frames get pumped on the main thread _without a transaction_
just before unmerging occurs (I don't know why this happens), and so
checking the current thread to determine whether we need to present with
a transaction or not isn't sufficient. In the prior fix, after the
unmerge, the raster thread would hang for one second while waiting for
the next drawable to get freed up (happens on the second raster thread
frame post-unmerge), and then subsequent presents would just do nothing
for a while, but eventually recover.

`presentsWithTransaction` works whether the `CATransaction` stack is
empty or not, and so the only difference here is that
`presentsWithTransaction` is always turned on and `presentDrawable` is
always avoided (otherwise it tries to present too early and nothing
renders when platform views are present).
@github-actions
Copy link

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 May 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@bdero bdero

Labels
e: impeller Impeller rendering backend issues and features requests engine flutter/engine repository. See also e: labels. P2 Important issues not at the top of the work list
Projects
No open projects
Impeller
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Impeller] iOS/macOS: Only wait for command scheduling prior to present (redux) bdero/flutter-engine
3 participants
@chinmaygarde @bdero @jonahwilliams