perf($compile): Lazily compile the transclude function
#12078
Conversation
|
This PR proposes a tradeoff Pro
Con
The PR still needs a few cleanups here and there and more than a few tests, but that aside, I like the idea proposed. @petebacondarwin WDYT for 1.5? |
| // is found, then we know that when we encounter a transcluded directive, we need to eagerly | ||
| // compile the `transclude` function rather than doing it lazily in order to throw | ||
| // exceptions at the correct time | ||
| if (!didScanForMultipleTransclusion && (directive.templateUrl && directive.replace) |
There was a problem hiding this comment.
Should this be !didScanForMultipleTransclusion && (a || b)? Right now it is !didScanForMultipleTransclusion && a || b...
There was a problem hiding this comment.
Absolutely, logic fail on my part.
|
Just to confirm... if you have an |
|
So the deferred loading of child templateUrl/async directives is something that I thought about quite a bit and figured would come up in this discussion. I think I agree that this will probably have to wait for another significant version bump. I think that is mitigated by two factors:
I avoided doing it in this pull request since I am not nearly familiar enough with the compile flow yet, but since the generated link functions are often long lived, I wonder there are references that can be nulled out to make them GC eligible once compilation for the directive is complete. |
|
@jbedard Precisely. I encountered this exact situation in an application that I support where one of the developers had written a giant ngSwitch statement ( like 20 cases ), and each of the cases had different templates to compile. By implementing this change, I brought the app startup and interactivity time from 20-30 seconds down to 1-2. At the moment, I'm having them work around the performance issue by manually using $compile with a real switch statement in another directive, but I feel like this concept would positively impact nearly all applications that use transclusion that isn't immediately called whether it's through ngIf, ngSwitch, custom directives, etc... |
|
@lgalfaso your point regarding templateUrl's is absolutely correct; however, even in this situation this is a balancing act because while you might save some async processing later, you're causing a flood of HTTP requests at load-time for all of the nested templates that may never even be needed. |
|
Maybe it's time for the giant directive loop in That would be a change in how error handling works though since it would throw before compiling anything instead of half way through. But that actually sounds better to me anyway... |
|
The only problem with that is that when we encounter the templateUrl + replace case, we can't synchronously know whether or not we will actually encounter the multiple transclusion error. I can very easily move the loop outside of the for loop, however I don't think I'd be able to maintain the exact same error messages as I did with this PR since The file could probably benefit from some maintenance though for sure. |
|
This is just a note that this PR is related to #9413. Lazy compilation of transcluded content was the original goal that led to discovery of that bug. There's a test here in the core suite that exercises it: https://github.com/angular/angular.js/blob/master/test/ng/compileSpec.js#L6064-L6077 And a directive here that has been used to improve performance as mentioned above: https://github.com/digitalbazaar/bedrock-angular-lazy-compile A very significant performance improvement was seen in start up time (and memory usage) for applications that include modals (or "sheets") that only need to be shown once activated. Note that the above directive also includes an experimental feature that attempts to cache the same transcluded content only once, further saving memory in certain situations. Having this sort of behavior on by default in angular core would be great. |
|
@dcherman we would like to get this into master and for it to make it into 1.5, but for this to happen it needs some tests and documentation changes. Would it be possible for you to add these to this PR? |
|
@lgalfaso Happy to do so. Other than the confirming that the new lazy behavior is working, were there any other tests that wanted added related to this? |
In all cases, using |
|
Working on this now. @lgalfaso out of curiosity, what's the timeline for 1.5? An alternative solution if you feels it's worthwhile could be to add |
|
@dcherman I would start by making this into 1.5, and then talk about back-porting this as something optional. The compiler core is very complex, and adding configurable behavior changes in this specific part of the code is not something I am very fond of. Anyhow, if there is enough interest and traction, then it is something that can be discussed. |
|
I've added some tests and am thinking about how to document some things. You'll notice that some tests for the eager cases weren't added; those code paths are already covered by earlier tests that check for multiple transclusion errors ( the reason eager loading was added in the first place ). I can add tests if you want, but they'll literally be copied and pasted from those earlier tests with the name of the test changed. |
| } | ||
| } | ||
|
|
||
| didScanForMultipleTransclusion = true; |
There was a problem hiding this comment.
I wonder if there is some corner case and we should add at line 1859
didScanForMultipleTransclusion = false?
This is, after a directive with a template and replace: true is found, then we might need to rescan once again
There was a problem hiding this comment.
Rather than doing that, template and replace: true should just be another case that can cause an eager compile if a second one is found. I'm not overly concerned about that path compiling eagerly since replace: true is deprecated
|
Overall, I like how this looks like. We should take a look at some of the most weird test cases, but overall, it looks fine. |
For transcluded directives, the transclude function can be lazily compiled most of the time since the contents will not be needed until the `transclude` function was actually invoked. For example, the `transclude` function that is passed to `ng-if` or `ng-switch-when` does not need to be invoked until the condition that it's bound to has been matched. For complex trees or switch statements, this can represent significant performance gains since compilation of branches is deferred, and that compilation may never actually happen if it isn't needed. There are two instances where compilation will not be lazy; when we scan ahead in the array of directives to be processed and find at least two of the following: * A directive that is transcluded and does not allow multiple transclusion * A directive that has templateUrl and replace: true * A directive that has a template and replace: true In both of those cases, we will need to continue eager compilation in order to generate the multiple transclusion exception at the correct time.
| * @param previousCompileContext | ||
| * @returns {Function} | ||
| */ | ||
| function compilationGenerator(eager, $compileNodes, transcludeFn, maxPriority, ignoreDirective, previousCompileContext) { |
There was a problem hiding this comment.
[Not a blocker to land this]
I am somehow torn on having all the parameters here or if we should use arguments. i.e.
function compilationGenerator(eager) {
var compileArguments = Array.slice(arguments);
compileArguments.shift();
[...]
return compile.apply(null, compileArguments);
[...]
}It is somehow more code, but it should be more resilient to changes. WDYT?
There was a problem hiding this comment.
If someone makes a change that makes the arguments passed invalid, we should have sufficient unit tests in order to catch any resulting bugs. Since the compile stage is generally considered a hot path, before making this change I'd be curious to see what ( if any ) perf implications there might be from V8 being unable to optimize this function due to the use of arguments like that.
We can also consider copying the arguments manually with a for loop which avoids the deopt if you think it really would make it more resilient.
|
Other that that, LGTM |
|
Will wait until we branch 1.4.x to land this. Thanks @dcherman ! |
|
landed as 652b83e |
|
Nice! Let's see what it breaks ;)
|
For transcluded directives, the transclude function can be lazily compiled
most of the time since the contents will not be needed until the
transcludefunction was actually invoked. For example, thetranscludefunction that is passed to
ng-iforng-switch-whendoes not need to beinvoked until the condition that it's bound to has been matched. For
complex trees or switch statements, this can represent significant
performance gains since compilation of branches is deferred, and that
compilation may never actually happen if it isn't needed.
In a large application where I work that recently had some very complex
ngSwitchcases added, this change significantly reduce the time spent in the compilation stage of the $digest cycle and further reduced the used JS heap size from 123MB to 56.8MB.There are two instances where compilation will not be lazy; when we scan
ahead in the array of directives to be processed and find at least two of
the following:
In both of those cases, we will need to continue eager compilation in
order to generate the multiple transclusion exception at the correct time. If anyone has a better idea on how to support this while maintaining backwards compatibility with existing unit tests, I'm all ears.
Fixes #6072