Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

perf($compile): only iterate over elements with link functions #8741

Closed
wants to merge 1 commit into from
+14 −13
Closed

perf($compile): only iterate over elements with link functions #8741

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 14 additions & 13 deletions src/ng/compile.js
View file
Original file line number Diff line number Diff line change
@@ -960,7 +960,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
function compileNodes(nodeList, transcludeFn, $rootElement, maxPriority, ignoreDirective,
previousCompileContext) {
var linkFns = [],
attrs, directives, nodeLinkFn, childNodes, childLinkFn, linkFnFound, nodeLinkFnFound;
attrs, directives, nodeLinkFn, childNodes, childLinkFn, nodeLinkFnFound;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that getting rid of the linkFnFound var makes things better/faster/more readable. I'd keep it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely not faster. I personally thought it was more readable but I'll revert it if you prefer the flag.


for (var i = 0; i < nodeList.length; i++) {
attrs = new Attributes();
@@ -987,38 +987,39 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
(nodeLinkFn.transcludeOnThisElement || !nodeLinkFn.templateOnThisElement)
&& nodeLinkFn.transclude) : transcludeFn);

linkFns.push(nodeLinkFn, childLinkFn);
linkFnFound = linkFnFound || nodeLinkFn || childLinkFn;
nodeLinkFnFound = nodeLinkFnFound || nodeLinkFn;
if (nodeLinkFn || childLinkFn) {
linkFns.push(i, nodeLinkFn, childLinkFn);
nodeLinkFnFound = nodeLinkFnFound || nodeLinkFn;
}

//use the previous context only for the first element in the virtual group
previousCompileContext = null;
}

// return a linking function if we have found anything, null otherwise
return linkFnFound ? compositeLinkFn : null;
return linkFns.length ? compositeLinkFn : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

revert


function compositeLinkFn(scope, nodeList, $rootElement, parentBoundTranscludeFn) {
var nodeLinkFn, childLinkFn, node, childScope, i, ii, n, childBoundTranscludeFn;
var nodeLinkFn, childLinkFn, node, childScope, i, ii, idx, childBoundTranscludeFn;
var stableNodeList;


if (nodeLinkFnFound) {
// copy nodeList so that if a nodeLinkFn removes or adds an element at this DOM level our
// offsets don't get screwed up
var nodeListLength = nodeList.length;
stableNodeList = new Array(nodeListLength);
stableNodeList = [];

for (i = 0; i < nodeListLength; i++) {
stableNodeList[i] = nodeList[i];
// create a sparse array by only copying the elements which have a linkFn
for (i = 0, ii = linkFns.length; i < ii; i+=3) {
idx = linkFns[i];
stableNodeList[idx] = nodeList[idx];
Copy link
Contributor

Choose a reason for hiding this comment

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

you are creating sparse arrays here. that will disable all major array optimizations in the VM. we need to avoid that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could just revert this block then. Or could the sparse array lookups be worth the shorter array and loop?

Or make stableNodeList non-sparse by just pushing the nodes onto it. Then we'd need another array mapping stableNodeList index => linkFn index? Sounds ugly...

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought about this more and getting rid of the sparse array is not that easy as it makes code in the later loop more complicated. we should at least prealocate the array as we used to.

}
} else {
stableNodeList = nodeList;
}

// TODO(perf): when the DOM is sparsely annotated with directives, we spend a lot of time iterating over nulls here
for(i = 0, n = 0, ii = linkFns.length; i < ii; n++) {
node = stableNodeList[n];
for(i = 0, ii = linkFns.length; i < ii; ) {
node = stableNodeList[ linkFns[i++] ];
nodeLinkFn = linkFns[i++];
childLinkFn = linkFns[i++];