Optimize ContentLayers._isSubtreeDirty

[knopp]

I was investigating excessive garbage collection when changing selection in documents with large number of tasks.

Much of the garbage seems to be generated from isSubtreeDirty, which is used to traverse, in this instance, a very large widget tree.

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

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

There are two issues here. Interpolating the log string (and serializing the diagnosticable nodes) and the closure allocated for each recursive call. The string interpolation can be put behind a log level check and ideally only do for debug build, since two extra branches can give a lot of overhead and the closure allocation can be avoided by using a static method.

With this PR the garbage collection pretty much disappears from the profile:

[knopp]

cc @matthew-carroll

[matthew-carroll]

Please order method declarations in the order that they're used. In this case _isSubtreeDirty() before _isSubtreeDirtyVisitor().

PS - Typically we place all static members above all instance members. However, in this very unusual situation, we'll treat the static property and method as instance members because we're working around a language issue.

[knopp]

Where should the static variable go?

[matthew-carroll]

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

[knopp]

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).

[matthew-carroll]

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

[knopp]

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).

[knopp]

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.

[matthew-carroll]

@knopp - Can you check the failing analysis? I'm guessing it's some new lint check that's only on master? I'm not sure.

[knopp]

@knopp - Can you check the failing analysis? I'm guessing it's some new lint check that's only on master? I'm not sure.

Will do.

[knopp]

@matthew-carroll, I moved the visitor method below and the PR to fix the analyser complaining is here.

[knopp]

Friendly ping, @matthew-carroll, does this need anything else?

[matthew-carroll]

LGTM

[matthew-carroll]

@angelosilvestre can you cherry pick this?