perf($compile): replace forEach(controller) with plain loops #11084
Conversation
jbedard
force-pushed
the
compile-controller-perf-foreach
branch
from
8925652 to
77615b0
Compare
| @@ -1877,6 +1877,36 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { | |||
| return value || null; | |||
| } | |||
|
|
|||
| function setupControllers($element, attrs, transcludeFn, controllerDirectives, isolateScope, scope) { | |||
There was a problem hiding this comment.
by adding newIsolateScopeDirective as a parameter, this can be moved out of the closure (or I am missing something?)
There was a problem hiding this comment.
We would also need access to hasElementTranscludeDirective. Let's not go there for now...
| for (var controllerKey in controllerDirectives) { | ||
| var directive = controllerDirectives[controllerKey]; | ||
| var locals = { | ||
| $scope: directive === newIsolateScopeDirective || directive.$$isolateScope ? isolateScope : scope, |
There was a problem hiding this comment.
This can just be
$scope: isolateScope || scope,right?
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Doh! Sorry. Yes, you are right - I didn't notice the comparison with directive.
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
nodeLinkFna bit...(this was originally part of #9772)