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

perf($compile): replace forEach(controller) with plain loops #11084

Closed
wants to merge 1 commit into from
Closed

perf($compile): replace forEach(controller) with plain loops #11084

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
70 changes: 38 additions & 32 deletions src/ng/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1671,7 +1671,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

if (!directive.templateUrl && directive.controller) {
directiveValue = directive.controller;
controllerDirectives = controllerDirectives || {};
controllerDirectives = controllerDirectives || createMap();
assertNoDuplicate("'" + directiveName + "' controller",
controllerDirectives[directiveName], directive, $compileNode);
controllerDirectives[directiveName] = directive;
Expand Down Expand Up @@ -1877,6 +1877,36 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
return value || null;
}

function setupControllers($element, attrs, transcludeFn, controllerDirectives, isolateScope, scope) {
Copy link
Contributor

Choose a reason for hiding this comment

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

by adding newIsolateScopeDirective as a parameter, this can be moved out of the closure (or I am missing something?)

Copy link
Contributor

Choose a reason for hiding this comment

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

We would also need access to hasElementTranscludeDirective. Let's not go there for now...

var elementControllers = createMap();
for (var controllerKey in controllerDirectives) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not a big fan of for ... in, would like to know more opinions

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, angular.forEach ends up using for ... in in any case, except that it checks the hasOwnProperty first. In this case, I am pretty confident that controllerDirectives will not contain any dodgy properties, especially since @jbedard has used createMap() to ensure that it doesn't derive from Object.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Personally I often prefer for ... in over forEach. That's all forEach does anyway.

Could use hasOwnProperty instead of createMap but that seems to be slower in many cases. Although that would actually make it more like previously (because that's what forEach does) which would reduce the changes in this commit.

The other option is Object.keys() + for (0 to len). Or maybe controllerDirectives could actually be an array? Looks like the only place controllerDirectives is used other then looping over the values is for the assertNoDuplicate check...

Copy link
Contributor

Choose a reason for hiding this comment

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

I am happy with for ... in in this case where we have control over the object being iterated over.

var directive = controllerDirectives[controllerKey];
var locals = {
$scope: directive === newIsolateScopeDirective || directive.$$isolateScope ? isolateScope : scope,
Copy link
Contributor

Choose a reason for hiding this comment

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

This can just be

$scope: isolateScope || scope,

right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think so... we only want the isolated scope if this controller is for the isolated scope directive. There might be other controllers that need to be initialized with the normal scope...

Copy link
Contributor

Choose a reason for hiding this comment

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

But the only place that isolateScope gets assigned is line 1925 (https://github.com/angular/angular.js/pull/11084/files#diff-a732922b631efed1b9f33a24082ae0dbR1924), which only happens if newIsolateScopeDirective is truthy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But that doesn't mean directive === newIsolateScopeDirective, there might be other controllers that don't want the isolated scope. Unless directive.$$isolateScope will always be true on that case? (which I'm assuming it is not)

Copy link
Contributor

Choose a reason for hiding this comment

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

Doh! Sorry. Yes, you are right - I didn't notice the comparison with directive.

$element: $element,
$attrs: attrs,
$transclude: transcludeFn
};

var controller = directive.controller;
if (controller == '@') {
controller = attrs[directive.name];
}

var controllerInstance = $controller(controller, locals, true, directive.controllerAs);

// For directives with element transclusion the element is a comment,
// but jQuery .data doesn't support attaching data to comment nodes as it's hard to
// clean up (http://bugs.jquery.com/ticket/8335).
// Instead, we save the controllers for the element in a local hash and attach to .data
// later, once we have the actual element.
elementControllers[directive.name] = controllerInstance;
if (!hasElementTranscludeDirective) {
$element.data('$' + directive.name + 'Controller', controllerInstance.instance);
}
}
return elementControllers;
}

function nodeLinkFn(childLinkFn, scope, linkNode, $rootElement, boundTranscludeFn,
thisLinkFn) {
Expand All @@ -1903,32 +1933,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
}

if (controllerDirectives) {
elementControllers = {};
forEach(controllerDirectives, function(directive) {
var locals = {
$scope: directive === newIsolateScopeDirective || directive.$$isolateScope ? isolateScope : scope,
$element: $element,
$attrs: attrs,
$transclude: transcludeFn
}, controllerInstance;

controller = directive.controller;
if (controller == '@') {
controller = attrs[directive.name];
}

controllerInstance = $controller(controller, locals, true, directive.controllerAs);

// For directives with element transclusion the element is a comment,
// but jQuery .data doesn't support attaching data to comment nodes as it's hard to
// clean up (http://bugs.jquery.com/ticket/8335).
// Instead, we save the controllers for the element in a local hash and attach to .data
// later, once we have the actual element.
elementControllers[directive.name] = controllerInstance;
if (!hasElementTranscludeDirective) {
$element.data('$' + directive.name + 'Controller', controllerInstance.instance);
}
});
elementControllers = setupControllers($element, attrs, transcludeFn, controllerDirectives, isolateScope, scope);
}

if (newIsolateScopeDirective) {
Expand Down Expand Up @@ -1958,17 +1963,18 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
bindings, scopeDirective);
}
}
forEach(elementControllers, function(controller) {
var result = controller();
if (result !== controller.instance &&
for (i in elementControllers) {
controller = elementControllers[i];
var controllerResult = controller();
if (controllerResult !== controller.instance &&
controller === controllerForBindings) {
// Remove and re-install bindToController bindings
thisLinkFn.$$destroyBindings();
thisLinkFn.$$destroyBindings =
initializeDirectiveBindings(scope, attrs, result,
initializeDirectiveBindings(scope, attrs, controllerResult,
bindings, scopeDirective);
}
});
}
}

// PRELINKING
Expand Down