-
Notifications
You must be signed in to change notification settings - Fork 6k
[Impeller] Use NSAutoReleasePool for Metal playgrounds. #40748
Conversation
Fixes flutter/flutter#117339 Avoids creating a new parallel ObjC++ TU
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. |
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.
Oh, makes sense! Could this also explain the leak of the command buffers? The buffers hold onto resources and that could explain the massive leaks. Perhaps we can back out that commit now.
I also see a scoped NSObject for autorelease pools in fml. But the implementation is missing and the comment mentions a missing alternative. In a separate patch, we should clean that up move these utilities to FML.
Either way, lgtm. This is a massive improvement to the playgrounds experience.
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
The command buffer leak is a separate issue - applying this patch doesn't seem to help it (they're actual leaks, whereas this is just flushing the autorelease pool on each turn of the loop) |
Fixes flutter/flutter#117339
I judged this better than having a separate ObjC++ implementation for macOS playgrounds.