[Impeller] started cleaning up pools when devices are deleted #41857
Conversation
| }); | ||
| return fence_waiter_->AddFence(std::move(fence), | ||
| [tracked_objects = std::move(tracked_objects_), | ||
| strong_device = std::move(strong_device)] { |
There was a problem hiding this comment.
Why is this capturing a strong reference to the ContextVK in the lambda?
TrackedObjectsVK holds a weak reference to the CommandPoolVK. That should allow cleanup of the tracked objects if the CommandPoolVK and the ContextVK/vk::Device are still alive (and do nothing if they are already gone)
There was a problem hiding this comment.
It's because that buffer.release() call that you just added wasn't in the TrackedObjects destructor. This may no longer be necessary after your change. Let me test it.
There was a problem hiding this comment.
Nope, is still necessary. It crashes without it because VALIDATION_LOG has an abort in it. I'll remove it.
There was a problem hiding this comment.
Our options are to keep a strong hold on the device in the fence, or to release the buffer without crashing. I believe you prefer removing the strong link and calling release(). I think I do too so I did that, although I'm unhappy about the C++ vulkan wrapper and how it looks like a leak.
|
|
||
| void CommandPoolVK::ClearAllPools(const ContextVK* context) { | ||
| if (tls_command_pool.get()) { | ||
| tls_command_pool.get()->erase(context->GetHash()); |
There was a problem hiding this comment.
Move this to the end of the function so it runs after the block that calls Reset on every CommandPoolVK associated with this context.
In theory this should be unnecessary because nobody else should be holding on to a strong reference to this thread's CommandPoolVK, and the erase should destruct it and do all the necessary cleanup. But for safety I think if would be prudent to ensure that the current thread's CommandPoolVK for this context is explicitly reset at the same time as the ones for other threads.
There was a problem hiding this comment.
Actually I just realized this is unnecessary. If this thread's CommandPoolVK is still alive after the tls_command_pool.erase, then the corresponding weak_ptr in g_all_pools will still be promotable to a shared_ptr and will get the Reset call.
So the current implementation in the PR will work as intended.
gaaclarke
added
the
autosubmit
…ions) (#126445) Manual roll requested by [email protected] flutter/engine@78f41a8...10ac36c 2023-05-09 [email protected] Implement text rendering in Skwasm (flutter/engine#41832) 2023-05-09 [email protected] Roll Fuchsia Linux SDK from leCRDVJ8szOS2LsPV... to lf8VcONMWlne4oa3H... (flutter/engine#41876) 2023-05-09 [email protected] [Impeller] started cleaning up pools when devices are deleted (flutter/engine#41857) 2023-05-09 [email protected] Roll Fuchsia Mac SDK from c4JvQUEOBHgtRdNPc... to JiOACcaGrDphuHIql... (flutter/engine#41874) 2023-05-09 [email protected] Move linux_license to engine v2. (flutter/engine#41863) 2023-05-09 [email protected] Replace Windows Unopt with the engine v2 version. (flutter/engine#41861) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from leCRDVJ8szOS to lf8VcONMWlne fuchsia/sdk/core/mac-amd64 from c4JvQUEOBHgt to JiOACcaGrDph 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
…ions) (flutter#126445) Manual roll requested by [email protected] flutter/engine@78f41a8...10ac36c 2023-05-09 [email protected] Implement text rendering in Skwasm (flutter/engine#41832) 2023-05-09 [email protected] Roll Fuchsia Linux SDK from leCRDVJ8szOS2LsPV... to lf8VcONMWlne4oa3H... (flutter/engine#41876) 2023-05-09 [email protected] [Impeller] started cleaning up pools when devices are deleted (flutter/engine#41857) 2023-05-09 [email protected] Roll Fuchsia Mac SDK from c4JvQUEOBHgtRdNPc... to JiOACcaGrDphuHIql... (flutter/engine#41874) 2023-05-09 [email protected] Move linux_license to engine v2. (flutter/engine#41863) 2023-05-09 [email protected] Replace Windows Unopt with the engine v2 version. (flutter/engine#41861) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from leCRDVJ8szOS to lf8VcONMWlne fuchsia/sdk/core/mac-amd64 from c4JvQUEOBHgt to JiOACcaGrDph 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
2 participants
fixes flutter/flutter#125571
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.