New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@jbedard
Contributor

jbedard commented Feb 17, 2015

Pretty minor but it is often a couple % spent just within these forEach(controller) calls. Minor perf aside, imo moving this chunk of code to it's own method is nicer anyway to reduce the size of nodeLinkFn a bit...

(this was originally part of #9772)

@googlebot googlebot added the cla: yes label Feb 17, 2015

@@ -1877,6 +1877,36 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
return value || null;
}
function setupControllers($element, attrs, transcludeFn, controllerDirectives, isolateScope, scope) {

This comment has been minimized.

@lgalfaso

lgalfaso Feb 17, 2015

Member

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

This comment has been minimized.

@petebacondarwin

petebacondarwin Feb 17, 2015

Member

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

@@ -1877,6 +1877,36 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
return value || null;
}
function setupControllers($element, attrs, transcludeFn, controllerDirectives, isolateScope, scope) {
var elementControllers = createMap();
for (var controllerKey in controllerDirectives) {

This comment has been minimized.

@lgalfaso

lgalfaso Feb 17, 2015

Member

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

This comment has been minimized.

@petebacondarwin

petebacondarwin Feb 17, 2015

Member

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.

This comment has been minimized.

@jbedard

jbedard Feb 17, 2015

Contributor

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

This comment has been minimized.

@petebacondarwin

petebacondarwin Feb 17, 2015

Member

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

for (var controllerKey in controllerDirectives) {
var directive = controllerDirectives[controllerKey];
var locals = {
$scope: directive === newIsolateScopeDirective || directive.$$isolateScope ? isolateScope : scope,

This comment has been minimized.

@petebacondarwin

petebacondarwin Feb 17, 2015

Member

This can just be

$scope: isolateScope || scope,

right?

This comment has been minimized.

@jbedard

jbedard Feb 17, 2015

Contributor

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

This comment has been minimized.

@petebacondarwin

petebacondarwin Feb 17, 2015

Member

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.

This comment has been minimized.

@jbedard

jbedard Feb 17, 2015

Contributor

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)

This comment has been minimized.

@petebacondarwin

petebacondarwin Feb 17, 2015

Member

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

netman92 added a commit to netman92/angular.js that referenced this pull request Aug 8, 2015

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