Material: Listening for DOMSubtreeModified during compilation dramatically impacts performance

[mtlewis]

The tabsTemplateDirective attaches an event listener for the DOMSubtreeModified event. Tabs with complex hierarchies of nested directives tend to modify the DOM a lot during compilation, so this event gets triggered a lot. Despite the fact that most of the time the handler isn't actually doing much due to the debounce, just the act of dispatching the event has a dramatic effect on performance (we saw an initial compilation go from 2-3 seconds down to around a second just by removing this handler, with no appreciable effects on functionality).

Is the attachment of this event handler something that can be delayed? It seems to only impact the initial compilation, so perhaps attaching the handler later on could result in the same behavior without the performance overhead?

[mtlewis]

Actually, I think it's probably fair to say that there shouldn't be a listener for this event at all, as the entire set of DOM mutation events is deprecated, and the performance implications of adding listeners for them are pretty well documented.

[ppalladino]

+1

[neoludo]

+1

[warpdesign]

+1
I'm having performance issues (that appear only when Chrome devtools are opened) and tracked it down to the tabs component.

I tried commenting the code that listens to the DOMSubtreeModified event and suddenly no more performance problems!

[Otanikotani]

_Copy of the description from the other bug:_
This is the code I use for tabs on my page:

        <md-tabs md-dynamic-height>
            <md-tab md-on-select="ctrl.$state.transitionTo('kpi.new.vehicle', {})"><md-tab-label>Vehicle</md-tab-label></md-tab>
            <md-tab md-on-select="ctrl.$state.transitionTo('kpi.new.dealer', {})"><md-tab-label>Dealer</md-tab-label></md-tab>
        </md-tabs>
        <div ui-view ng-cloak></div>

I profiled tabs switching from one to another:

As you can see on the pic, selected yellow area is the moment of tabs switching. It took about one second. Please, notice the DOMSubtreeModified events in the stacktrace. There are tons of them.

I think the issue is in this code, in the mdTabsTemplate directive:

    element.on('DOMSubtreeModified', function () {
      ctrl.updatePagination();
      ctrl.updateInkBarStyles();
    });

If I comment it out and make the same test:

Commenting out 'DOMSubtreeModified' event handler made about 0.8s difference in time - it is only 224ms now. Also notice that there are no DOMSubtreeModified in the stacktrace.

[warpdesign]

Is there a workaround other than commenting out the DOMSubtreeModified event handler for this problem ?

[christopheGaon]

+1

[ThomasBurleson]

@robertmesserle - think we need your eyes on this one!

[robertmesserle]

@ThomasBurleson @mtlewis This is a known performance hit; however, there's not really a clean workaround (at least not to my knowledge). Delaying the attachment may work to improve the performance (and should be fairly easy), but that's clearly a bandaid and not a proper fix.

[topherfangio]

Just got off a call with Robert where we talked about this in a bit more detail. Two potential options to investigate:

1. We may be able to mitigate the issue by binding to only the pagination wrapper element. Apparently, we used to use this notification to change the height if the content changed, but we now only use it to update the tabs and their pagination. This should significantly reduce the number of DOM modifications that we listen to.
2. We may be able to replace this with the new Mutation Observers, but Rob wasn't 100% sure, so we need to investigate.

I think a combination of both would be ideal and we will definitely look into it.

[jeff-mccoy]

I just swapped it out with a MutationObserver to compare and it resolved the issue for me. It ran only three times initially (vs several hundred).

      new MutationObserver(function() {
          ctrl.updatePagination();
          ctrl.updateInkBarStyles();
      }).observe(element[0], { childList: true, subtree: true } );

[topherfangio]

@jeff-mccoy Thanks for testing that out! Any chance you could make a PR with some tests for it?

[jeff-mccoy]

@topherfangio Sorry I've been out of town, I'll look into writing a test for this, but it looks like there is no test coverage for the existing DOM listener. Also, the change doesn't seem to affect any other tests.

Honestly, I'm still trying to understand the need for the binding. I see where it was put in f5959cc2e1d3a960c8df344669731c1a16c3e617. It was originally introduced in 8ac4fa1b3f23f1f73125cc7e2425d349d7ad546e by @robertmesserle, so maybe he could shed some light on it.

[topherfangio]

@jeff-mccoy I believe the point of the mutation observer/binding is to re-draw/re-paginate when a tab's title changes. If it gets longer, then that will change the overall width of the tabs and so we need to update the pager so it recalculates properly.