Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Optimize ContentLayers._isSubtreeDirty #1964

Merged
merged 3 commits into from
Apr 29, 2024
Merged

Optimize ContentLayers._isSubtreeDirty #1964

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0e9a266
Optimize ContentLayers._isSubtreeDirty
knopp Apr 23, 2024
8f66646
Only log in debug build.
knopp Apr 23, 2024
47cdad4
Move _isSubtreeDirty visitor below
knopp Apr 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 29 additions & 11 deletions super_editor/lib/src/infrastructure/content_layers.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import 'package:flutter/foundation.dart';
import 'package:flutter/rendering.dart';
import 'package:flutter/scheduler.dart';
import 'package:flutter/widgets.dart';
import 'package:logging/logging.dart';
import 'package:super_editor/src/infrastructure/_logging.dart';

/// Widget that displays [content] above a number of [underlays], and beneath a number of
Expand Down Expand Up @@ -208,31 +209,48 @@ class ContentLayersElement extends RenderObjectElement {

contentLayersLog.finer("Checking underlays");
for (final underlay in _underlays) {
contentLayersLog.finer(" - Is underlay ($underlay) subtree dirty? ${_isSubtreeDirty(underlay)}");
contentLayersLog.finer(() => " - Is underlay ($underlay) subtree dirty? ${_isSubtreeDirty(underlay)}");
hasDirtyElements = hasDirtyElements || _isSubtreeDirty(underlay);
}

contentLayersLog.finer("Checking overlays");
for (final overlay in _overlays) {
contentLayersLog.finer(" - Is overlay ($overlay) subtree dirty? ${_isSubtreeDirty(overlay)}");
contentLayersLog.finer(() => " - Is overlay ($overlay) subtree dirty? ${_isSubtreeDirty(overlay)}");
hasDirtyElements = hasDirtyElements || _isSubtreeDirty(overlay);
}

return hasDirtyElements;
}

static bool _isDirty = false;

bool _isSubtreeDirty(Element element) {
contentLayersLog.finest("Finding dirty children for: $element");
if (element.dirty) {
contentLayersLog.finest("Found a dirty child: $element");
_isDirty = false;
element.visitChildren(_isSubtreeDirtyVisitor);
return _isDirty;
}

// This is intentionally static to prevent closure allocation during
// the traversal of the element tree.
static void _isSubtreeDirtyVisitor(Element element) {
// Can't use the () => message syntax because it allocates a closure.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If () => allocates a closure, and that's harmful to performance, then why did you use that syntax up above?

Copy link
Contributor Author

@knopp knopp Apr 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because there's several orders of magnitude less overlay / underlay layers than the widgets in document. In this case I think we have over 100000 elements in document all being traversed. If you want me to I can do the same thing above (for consistency).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw. I introduced the () => closure to prevent diagnosticable tree creates in debug mode during every call (closures in log are only evaluated when log lovel is reached), which is quite slow especially in debug mode.

assert(() {
if (contentLayersLog.isLoggable(Level.FINEST)) {
contentLayersLog.finest("Finding dirty children for: $element");
}
return true;
}());
if (element.dirty) {
assert(() {
if (contentLayersLog.isLoggable(Level.FINEST)) {
contentLayersLog.finest("Found a dirty child: $element");
}
return true;
}());
_isDirty = true;
return;
}

bool isDirty = false;
element.visitChildren((childElement) {
isDirty = isDirty || _isSubtreeDirty(childElement);
});
return isDirty;
element.visitChildren(_isSubtreeDirtyVisitor);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised the compiler wasn't already in-lining this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an indirect call to a closure or function pointer passed to a virtual method. I don't think it's really possible to inline this. The call overhead is much less of a problem than the need to allocate closure that references local variable (which on top of it is modified from closure so itself needs to be boxed).

}

void _onContentBuildScheduled() {
Expand Down
Loading