Skip to content
This repository has been archived by the owner on Feb 22, 2018. It is now read-only.

Change the compiler to use template elements instead of comments for view ports #1365

Closed

Conversation

vsavkin
Copy link
Contributor

@vsavkin vsavkin commented Aug 18, 2014

DON'T MERGE

Using template elements makes viewports queryable, so we don't have to walk the DOM ourselves and rely on querySelectAll. It looks like the change causes a 10% performance degradation. This requires some investigation.

UPDATE:

I've written a new benchmark (#1384) to investigate this issue.

My findings:

The tree benchmark uses the following template:

<span> {{ctrl.data.value}}
  <span ng-if="ctrl.data.right != null"><tree data=ctrl.data.right></span>
  <span ng-if="ctrl.data.left != null"><tree data=ctrl.data.left></span>
</span>

Which gets compiled to:

<span> {{ctrl.data.value}}
  <!--ANCHOR: [ng-if]=ctrl.data.right != null-->
  <!--ANCHOR: [ng-if]=ctrl.data.left != null-->
</span>

There are not elements with the ng-binding class in this template, which means that ViewFactory does not have to call querySelectAll.

After replacing the comment element with the template element, you get:

<span> {{ctrl.data.value}}
  <template type="ng/ViewPort" value="ctrl.data.right != null" class="ng-binding"/>
  <template type="ng/ViewPort" value="ctrl.data.left != null" class="ng-binding"/>
</span>

This time you have two elements with the ng-binding class, which means that ViewFactory has to call querySelectAll.

Calling querySelectAll is expensive, and that is why compiling the second template is about 30% slower (which causes a 10% performance degradation).

If you, however, change the template to:

<span> {{ctrl.data.value}} <span ng-class="{}"></span>
  <span ng-if="ctrl.data.right != null"><tree data=ctrl.data.right></span>
  <span ng-if="ctrl.data.left != null"><tree data=ctrl.data.left></span>
</span>

Then ViewFactory will have to call querySelectAll in both cases, and the template option becomes a tiny bit faster (about 4%), which has almost no effect on the overall performance.

@@ -329,6 +329,11 @@ class ElementBinder {
return nodeInjector;
}

bool _viewPort(dom.Node node) =>
(node is dom.TemplateElement &&
Copy link
Contributor

Choose a reason for hiding this comment

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

all 3 conditions could be aligned, no need for external ()

@jbdeboer
Copy link
Contributor

@vsavkin Could you make a go/no-go call for this PR?

@vsavkin
Copy link
Contributor Author

vsavkin commented Aug 26, 2014

This PR causes a 10% performance degradation for a certain class of templates, and it only marginally improves the performance of others.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

4 participants