Adds API for retaining intermediate engine layers #9461
Conversation
|
/cc @jonahwilliams |
testing/dart/compositing_test.dart
Outdated
| } | ||
| builder2.build(); | ||
|
|
||
| // Test: first addRetained then attempt illegal push |
There was a problem hiding this comment.
Maybe also test two pushes with the same oldLayer?
There was a problem hiding this comment.
Good idea! Done. Also added two addRetaineds.
| // | ||
| // The key is the layer used. The value is the description of what the layer | ||
| // is used for, e.g. "pushOpacity" or "addRetained". | ||
| Map<EngineLayer, String> _usedLayers = <EngineLayer, String>{}; |
There was a problem hiding this comment.
Could we also catch the case where a layer is marked an oldLayer, and thus freed, but then it's reused in another scene builder?
There was a problem hiding this comment.
That's actually a supported case in Flutter for web. The old layer becomes "released" and when you use it again the engine code will "revive" it. Other than the DOM nodes the information stored on the layers is immutable and so we can always re-inflate a DOM tree from discarded layers.
The usage you describe was not why we implemented it though. We implemented it because you can push a new layer in builder1, then not use that layer in builder2 (thus another layer could "adopt" its DOM nodes), and call addRetained or pass it as oldLayer in builder3. We even have a test for this.
There was a problem hiding this comment.
The test/use case you referred seems reasonable. But is it legal to do the following?
- Create a
layer1in frame/builder 1, - Call
pushLayer(..., oldLayer: layer1)in frame/builder 2, - Call
addRetained(layer1)in frame/builder 3
That sounds a little counter-intuitive and some engine implementation may view that as illegal. If we want to make this kind of usages legal, maybe it's worth to document it, and put a unit test here to ensure that all engine implementations can support it.
There was a problem hiding this comment.
Intuitively you can think of it as either of the following two ways, depending on whether the layers are immutable or mutable:
- Immutable: an
EngineLayeris simply a description of what should appear in a given subtree of a scene. It should be OK to go back and forth between two descriptions, all while updating existing underlying resources. - Mutable:
pushLayermay actually return the same object asoldLayer. Since the object is the same, you'd have to pass it asoldLayermultiple times to communicate an "update".
There was a problem hiding this comment.
However, I don't mind putting a restriction in place. I don't think we are using layers in a way where this matters today. LMK which way you prefer.
There was a problem hiding this comment.
Let's restrict it now as I think it's easier to add capabilities than to remove capabilities that are being used :)
Done (added links to the PR description) |
|
PTAL. |
lib/ui/compositing.dart
Outdated
| 'A layer may only be used once in a given scene.' | ||
| ); | ||
|
|
||
| _usedLayers[layer] = usage; |
There was a problem hiding this comment.
this whole method should be in an assert, to make sure we really don't use it in release
lib/ui/compositing.dart
Outdated
| void addRetained(EngineLayer retainedLayer) native 'SceneBuilder_addRetained'; | ||
| void addRetained(EngineLayer retainedLayer) { | ||
| assert(retainedLayer is _EngineLayerWrapper); | ||
| assert(_debugCheckUsedOnce(retainedLayer, 'addRetained')); |
There was a problem hiding this comment.
this also needs to add all the descendants of the retained layer
There was a problem hiding this comment.
Ah, you're right. Done.
|
I added more tests and the "oldLayers can't be reused" restriction that @liyuqian asked for. PTAL. |
lib/ui/compositing.dart
Outdated
| @@ -249,6 +249,23 @@ class SceneBuilder extends NativeFieldWrapperClass2 { | |||
| /// an optimization. It has no effect on the correctness of rendering. | |||
| /// {@endtemplate} | |||
| /// | |||
| /// {@template dart.ui.sceneBuilder.oldLayerVsRetained} | |||
| /// Passing a layer to [addRetained] or as `oldLayer` argument to a push | |||
| /// method counts as _usage_. A layer be used no more than once in a scene. | |||
There was a problem hiding this comment.
Nit: "A layer be" -> "A layer can be"?
lib/ui/compositing.dart
Outdated
| /// | ||
| /// When a layer is passed as an `oldLayer` argument to a push method, it may | ||
| /// no longer be used in subsequent frames. If you would like to continue | ||
| /// reusing the resources associated layer, store the layer object returned |
There was a problem hiding this comment.
Nit: "associated layer" -> "associated with the layer"?
yjbanov
force-pushed
the
intermediate-layers
branch
from
7659a46 to
0e62b95
Compare
Add new optional named
oldLayerarguments to allpush*methods of theSceneBuilderclass.The Web changes are temporary. We will get those from
package:flutter_web_uiseparately.When not null
oldLayersignals to the engine that the intent is to update a layer rendered in a previous frame. The engine may optionally use that signal to reuse the resources allocated for that layer in the previous frame. For example, on the Web we can reuse existing DOM nodes and some of their properties and move fewer nodes around the tree.The return type of each push method has been tightened up. Instead of having all methods return the same
EngineLayertype, each method has its own unique layer type, e.g.OffsetEngineLayer.oldLayerparameters match the returned type. This prevents the framework (and other developers usingdart:uidirectly) from accidentally supplying an engine layer of the wrong type.Here are some pointers to Flutter for web where
oldLayeris used to reuse DOM nodes:pendingUpdate,isPendingUpdate, andoldLayer.Breaking changes:
SceneBuildermust add the newoldLayerparameter and tighten the return types of their push method.