-
-
Notifications
You must be signed in to change notification settings - Fork 1k
perf: Cache decorator and camera render closures instead of allocating per frame #3979
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -162,10 +162,14 @@ class Route extends PositionComponent | |
| } | ||
| } | ||
|
|
||
| /// Cached `super.renderTree` tear-off, so that the render pass does not | ||
| /// allocate a fresh closure for [Decorator.applyChain] on every frame. | ||
| void Function(Canvas)? _superRenderTree; | ||
|
|
||
| @override | ||
| void renderTree(Canvas canvas) { | ||
| if (isRendered) { | ||
| _renderEffect.applyChain(super.renderTree, canvas); | ||
| _renderEffect.applyChain(_superRenderTree ??= super.renderTree, canvas); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it is quite unfortunate that the compiler can't optimize this
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. an alternative to consider, make applyChain receive a generic component or superclass with renderTree and have it call the function directly, avoiding the lambda entirely |
||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -105,22 +105,25 @@ class PostProcessComponent<T extends PostProcess> extends PositionComponent { | |
| return superSize; | ||
| } | ||
|
|
||
| /// Cached render chain, so that the render pass does not allocate fresh | ||
| /// closures for `Decorator.applyChain` on every frame. | ||
| void Function(Canvas)? _renderChain; | ||
|
|
||
| @override | ||
| @mustCallSuper | ||
| void renderTree(Canvas canvas) { | ||
| decorator.applyChain( | ||
| (canvas) { | ||
| postProcess.render( | ||
| canvas, | ||
| size, | ||
| super.renderTreeWithoutDecorator, | ||
| (context) { | ||
| _renderContext.postProcess = postProcess; | ||
| }, | ||
| ); | ||
| }, | ||
| canvas, | ||
| ); | ||
| decorator.applyChain(_renderChain ??= _buildRenderChain(), canvas); | ||
| } | ||
|
|
||
| void Function(Canvas) _buildRenderChain() { | ||
| final renderTree = super.renderTreeWithoutDecorator; | ||
| void updateContext(PostProcess? context) { | ||
| _renderContext.postProcess = postProcess; | ||
| } | ||
|
|
||
| return (canvas) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you don't want to tear-off-ify this lambda too? |
||
| postProcess.render(canvas, size, renderTree, updateContext); | ||
| }; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,16 +31,26 @@ class Decorator { | |
| /// The next decorator in the chain, or null if there is none. | ||
| Decorator? _next; | ||
|
|
||
| /// Cached closure that forwards the draw call to the rest of the chain, | ||
| /// so that no closure needs to be allocated per frame. It is keyed by the | ||
| /// identity of the [_chainedDrawSource] it wraps: callers that pass the | ||
| /// same (cached) draw callback every frame reuse the same chain closure. | ||
| late void Function(Canvas) _chainedDraw; | ||
| void Function(Canvas)? _chainedDrawSource; | ||
|
|
||
| /// Applies this and all subsequent decorators if any. | ||
| /// | ||
| /// This method is the main method through which the decorator is applied. | ||
| void applyChain(void Function(Canvas) draw, Canvas canvas) { | ||
| apply( | ||
| _next == null | ||
| ? draw | ||
| : (nextCanvas) => _next!.applyChain(draw, nextCanvas), | ||
| canvas, | ||
| ); | ||
| if (_next == null) { | ||
| apply(draw, canvas); | ||
| } else { | ||
| if (!identical(_chainedDrawSource, draw)) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the need for this makes me question if the decorator pattern is the best one for the dart language. |
||
| _chainedDrawSource = draw; | ||
| _chainedDraw = (nextCanvas) => _next!.applyChain(draw, nextCanvas); | ||
| } | ||
| apply(_chainedDraw, canvas); | ||
| } | ||
| } | ||
|
|
||
| /// Applies visual effect while [draw]ing on the [canvas]. | ||
|
|
||
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.
curious why all the others can't use the has decorator mixin? is there a way to share this logic upstream?