[Impeller] FenceWaiterVK: Move potentially slow calls out of wait set mutex critical section. #43131
Conversation
… mutex critical section. Moves signaled checks and callbacks out of the wait set mutex critical section. The signaled checked can apparently be super slow on Qualcomm devices and the callback may hit allocators that could be slow as well.
|
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. |
jonahwilliams
left a comment
jonahwilliams
left a comment
There was a problem hiding this comment.
LGTM
Love reading your PRs, I get to learn more C++
TIL about copy_if!
| std::scoped_lock lock(wait_set_mutex_); | ||
| std::copy_if(wait_set_.begin(), wait_set_.end(), | ||
| std::back_inserter(erased_entries), is_signalled); | ||
| wait_set_.erase( |
There was a problem hiding this comment.
Aren't you still calling the callback in the while inside the lock? The last reference deleted should be here (inside of a lock). On the other PR you said you wanted to remove the callback outside of the lock.
There was a problem hiding this comment.
Probably the easiest fix is to make UpdateSignalStatus call the callback.
This code would probably be easier to follow if it was converted to use std::unique_ptr then holding raw pointers on the stack in wait_set.
There was a problem hiding this comment.
The entries are shared pointers to WaitSetEntry. These have been copied over to the erased_entries set. That set is cleared outside the critical section. The scoped cleanup closures get dropped in WaitSetEntry dtor.
There was a problem hiding this comment.
Ahh, I missed erased_entries. LGTM.
| { | ||
| TRACE_EVENT0("impeller", "ClearSignaledFences"); | ||
| // Erase the erased entries which will invoke callbacks. | ||
| erased_entries.clear(); // Bit redundant because of scope but hey. |
There was a problem hiding this comment.
@gaaclarke Here is where the callbacks are invoked.
"105 STL Algorithms in Less Than an Hour" is not a bad watch. |
chinmaygarde
added
the
autosubmit
…f wait set mutex critical section. (flutter/engine#43131)
…129445) flutter/engine@74ef618...f8a39cb 2023-06-23 [email protected] Roll ANGLE from 2e285bb591f7 to bc2d5ed01f27 (1 revision) (flutter/engine#43151) 2023-06-23 [email protected] [Impeller] FenceWaiterVK: Move potentially slow calls out of wait set mutex critical section. (flutter/engine#43131) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
3 participants
Moves signaled checks and callbacks out of the wait set mutex critical section.
The signaled checked can apparently be super slow on Qualcomm devices and the callback may hit allocators that could be slow as well.