-
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
EngineLayer::dispose #26219
EngineLayer::dispose #26219
Conversation
720537d
to
a4fdb7c
Compare
@goderbauer FYI since I'll be asking you to review framework side changes related to this. |
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
lib/ui/compositing.dart
Outdated
|
||
@override | ||
void dispose() { | ||
assert(!_debugDisposed); |
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.
Would it make sense to set _nativeLayer
to null after disposing instead of maintaining the _debugDisposed
flag?
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 guess we could make it nullable and avoid the flag. I can change that.
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.
Done - I was worried this would add a ton of null checks, but we basically treat it as nullable in most places anyway.
@dnfield Is there a better view of how this will be integrated into the framework? The diff posted in the issue (https://github.com/flutter/flutter/compare/master..dnfield:disp_pict) seems to contain a lot of unrelated stuff... |
@goderbauer - everything in that diff is related, even if it seems like it's not :. I'll be splitting that up into the render object portion and the layer portion soon though. |
That looks better. Added to my queue. |
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 lgtm. @yjbanov and @chinmaygarde should review as well.
One thing I want to think about some more is what to do with |
This pull request is not suitable for automatic merging in its current state.
|
Part of flutter/flutter#81514
In effect, we end up with three layer trees:
SceneBuilder
. Those layers may or may not hold refernces toEngineLayer
s. Some framework layers have no corresponding engine layer even though they have a corresponding flow layer (e.g. PictureLayer). Some have no corresponding engine or flow layer.EngineLayer
objects. It may or may not create additional layers that the framework doesn't know about. These layers contain pointers to other layers (child layers), and may also contain pointers to Skia objects (SkShader/SkPicture in particular).EngineLayer
tree, which links between framework and flow layers. It has no notions of parent/sibling/child relationships, and exists solely to help the framework with retained rendering.The main problem is that the linkage between the framework layers means that flow layers cannot be fully cleaned up until a GC is run. This in turn means that some Skia objects which are no longer necessary get stuck until a GC.
Once this patch has landed and the framework consumes the dispose API correctly, we won't need to trigger GCs on image disposal anymore. We will also much more efficiently clean up Skia and flow Layer* related memory before waiting for a GC, even with the engine holding on to the
last_layer_tree
at times.This still needs tests. Marking as draft to start running more tests than I've done locally.
Pre-launch Checklist
writing and running engine tests.
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.