-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
[Impeller] Started tracking the pool with the command buffer. #45298
Conversation
std::mutex reclaimables_mutex_; | ||
std::condition_variable reclaimables_cv_; | ||
Reclaimables reclaimables_; | ||
bool should_exit_ = false; | ||
// This should be initialized last since it references the other instance | ||
// variables. | ||
std::thread waiter_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a nasty race condition I was running into.
5ca3faa
to
1d680e4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defer explicit approval to someone else since I'm just getting started here, but thanks!
@chinmaygarde friendly ping |
namespace impeller { | ||
namespace testing { | ||
|
||
TEST(CommandEncoderVKTest, DeleteEncoderAfterThreadDies) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have sort of mixed feelings about these tests, because its sort disjointed from how these classes are used. I would feel better if there was substantially more commentary on specifically what these tests are covering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or at least for one of them, and then subsequent tests could have less commentary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unit tests are supposed to be testing how a class can be used, not how they are supposed to be used. Maybe it will help to understand why I implemented this fix. The pool change is a fix for a real bug that I was running into when using the playgrounds test harness which Chinmay was trying to get working. So, it may be true that this bug shouldn't present itself in production, it has demonstrably happened in our test harness and may present itself accidentally in production.
The second fix with the resourcemanager is definitely something that can manifest in production code, but was showing up regularly in these tests.
We typically don't annotate unit tests that are using the public api of a method. I'm happy to add any comments you think will help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that, what I'm saying is that I'm having a hard time following the test and so I'd appreciate more commentary on how/why the particular test successfully exercises the assertions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment to the first test, expanded the second comment and removed one piece of vestigial code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So today the current implementation with a weak_ptr is safe because we actually never actually clean up a cmd pool, we just recycle the same one over and over again.
Of course, if we start trying to recycle the cmd pools this might become a problem, but that problem might be different?
if the cmd encoder has a strong reference to the cmd pool, this won't cause retention cycles right? because the pool doesn't have a strong reference to the impeller::vk classes.
So thinking out loud I think this is safe
802956a
to
30b3b3d
Compare
This was manifesting in the playgrounds test harness, not in production apps. (mentioned above)
The other
Yep, I believe so. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Ask for comments is just a nit.
…134249) flutter/engine@2dba8ce...a828c26 2023-09-07 skia-flutter-autoroll@skia.org Roll ANGLE from 204c07a56b64 to cdbc45a9f37e (1 revision) (flutter/engine#45551) 2023-09-07 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from WbB3tmMXnuwJBAHoi... to EuBfOtm5TZIdgNaQe... (flutter/engine#45550) 2023-09-07 skia-flutter-autoroll@skia.org Roll Skia from ee741e5e8cf3 to ce2c94883cb5 (1 revision) (flutter/engine#45552) 2023-09-07 30870216+gaaclarke@users.noreply.github.com [Impeller] Started tracking the pool with the command buffer. (flutter/engine#45298) 2023-09-07 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 3.6.0 to 4.0.0 (flutter/engine#45439) 2023-09-07 41930132+hellohuanlin@users.noreply.github.com Reverts part of "fix auto-correction highlight on top left corner (Again)" (flutter/engine#45523) 2023-09-07 skia-flutter-autoroll@skia.org Roll Skia from 59a2610cd83d to ee741e5e8cf3 (4 revisions) (flutter/engine#45548) 2023-09-07 skia-flutter-autoroll@skia.org Roll ANGLE from 60b56591dee5 to 204c07a56b64 (7 revisions) (flutter/engine#45546) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from WbB3tmMXnuwJ to EuBfOtm5TZId 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 rmistry@google.com,zra@google.com 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
issue: flutter/flutter#133506
This fixes one of the issues seen in flutter/flutter#133506. There were multiple ones though.
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.