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

perf($compile): Lazily compile the transclude function #12078

Closed
wants to merge 1 commit into from
Closed

perf($compile): Lazily compile the transclude function #12078

Show file tree
Hide file tree
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
58 changes: 56 additions & 2 deletions src/ng/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1625,6 +1625,37 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
};
}

/**
* A function generator that is used to support both eager and lazy compilation
* linking function.
* @param eager
* @param $compileNodes
* @param transcludeFn
* @param maxPriority
* @param ignoreDirective
* @param previousCompileContext
* @returns {Function}
*/
function compilationGenerator(eager, $compileNodes, transcludeFn, maxPriority, ignoreDirective, previousCompileContext) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM

if (eager) {
return compile($compileNodes, transcludeFn, maxPriority, ignoreDirective, previousCompileContext);
}

var compiled;

return function() {
if (!compiled) {
compiled = compile($compileNodes, transcludeFn, maxPriority, ignoreDirective, previousCompileContext);

// Null out all of these references in order to make them eligible for garbage collection
// since this is a potentially long lived closure
$compileNodes = transcludeFn = previousCompileContext = null;
}

return compiled.apply(this, arguments);
};
}

/**
* Once the directives have been collected, their compile functions are executed. This method
* is responsible for inlining directive templates as well as terminating the application
Expand Down Expand Up @@ -1669,6 +1700,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
replaceDirective = originalReplaceDirective,
childTranscludeFn = transcludeFn,
linkFn,
didScanForMultipleTransclusion = false,
mightHaveMultipleTransclusionError = false,
directiveValue;

// executes all directives on the current element
Expand Down Expand Up @@ -1711,6 +1744,27 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

directiveName = directive.name;

// If we encounter a condition that can result in transclusion on the directive,
// then scan ahead in the remaining directives for others that may cause a multiple
// transclusion error to be thrown during the compilation process. If a matching directive
// 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.replace && (directive.templateUrl || directive.template))
|| (directive.transclude && !directive.$$tlb))) {
var candidateDirective;

for (var scanningIndex = i + 1; candidateDirective = directives[scanningIndex++];) {
if ((candidateDirective.transclude && !candidateDirective.$$tlb)
|| (candidateDirective.replace && (candidateDirective.templateUrl || candidateDirective.template ))) {
mightHaveMultipleTransclusionError = true;
break;
}
}

didScanForMultipleTransclusion = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, sounds good

}

if (!directive.templateUrl && directive.controller) {
directiveValue = directive.controller;
controllerDirectives = controllerDirectives || createMap();
Expand Down Expand Up @@ -1740,7 +1794,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
compileNode = $compileNode[0];
replaceWith(jqCollection, sliceArgs($template), compileNode);

childTranscludeFn = compile($template, transcludeFn, terminalPriority,
childTranscludeFn = compilationGenerator(mightHaveMultipleTransclusionError, $template, transcludeFn, terminalPriority,
replaceDirective && replaceDirective.name, {
// Don't pass in:
// - controllerDirectives - otherwise we'll create duplicates controllers
Expand All @@ -1754,7 +1808,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
} else {
$template = jqLite(jqLiteClone(compileNode)).contents();
$compileNode.empty(); // clear contents
childTranscludeFn = compile($template, transcludeFn);
childTranscludeFn = compilationGenerator(mightHaveMultipleTransclusionError, $template, transcludeFn);
}
}

Expand Down
207 changes: 207 additions & 0 deletions test/ng/compileSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6596,6 +6596,27 @@ describe('$compile', function() {
});
});

it('should only allow one element transclusion per element when replace directive is in the mix', function() {
module(function() {
directive('template', valueFn({
template: '<p second></p>',
replace: true
}));
directive('first', valueFn({
transclude: 'element',
priority: 100
}));
directive('second', valueFn({
transclude: 'element'
}));
});
inject(function($compile) {
expect(function() {
$compile('<div template first></div>');
}).toThrowMinErr('$compile', 'multidir', /Multiple directives \[first, second\] asking for transclusion on: <p .+/);
});
});


it('should support transcluded element on root content', function() {
var comment;
Expand Down Expand Up @@ -6892,6 +6913,192 @@ describe('$compile', function() {
});

});

it('should lazily compile the contents of directives that are transcluded', function() {
var innerCompilationCount = 0, transclude;

module(function() {
directive('trans', valueFn({
transclude: true,
controller: function($transclude) {
transclude = $transclude;
}
}));

directive('inner', valueFn({
template: '<span>FooBar</span>',
compile: function() {
innerCompilationCount +=1;
}
}));
});

inject(function($compile, $rootScope) {
element = $compile('<trans><inner></inner></trans>')($rootScope);
expect(innerCompilationCount).toBe(0);
transclude(function(child) { element.append(child); });
expect(innerCompilationCount).toBe(1);
expect(element.text()).toBe('FooBar');
});
});

it('should lazily compile the contents of directives that are transcluded with a template', function() {
var innerCompilationCount = 0, transclude;

module(function() {
directive('trans', valueFn({
transclude: true,
template: '<div>Baz</div>',
controller: function($transclude) {
transclude = $transclude;
}
}));

directive('inner', valueFn({
template: '<span>FooBar</span>',
compile: function() {
innerCompilationCount +=1;
}
}));
});

inject(function($compile, $rootScope) {
element = $compile('<trans><inner></inner></trans>')($rootScope);
expect(innerCompilationCount).toBe(0);
transclude(function(child) { element.append(child); });
expect(innerCompilationCount).toBe(1);
expect(element.text()).toBe('BazFooBar');
});
});

it('should lazily compile the contents of directives that are transcluded with a templateUrl', function() {
var innerCompilationCount = 0, transclude;

module(function() {
directive('trans', valueFn({
transclude: true,
templateUrl: 'baz.html',
controller: function($transclude) {
transclude = $transclude;
}
}));

directive('inner', valueFn({
template: '<span>FooBar</span>',
compile: function() {
innerCompilationCount +=1;
}
}));
});

inject(function($compile, $rootScope, $httpBackend) {
$httpBackend.expectGET('baz.html').respond('<div>Baz</div>');
element = $compile('<trans><inner></inner></trans>')($rootScope);
$httpBackend.flush();

expect(innerCompilationCount).toBe(0);
transclude(function(child) { element.append(child); });
expect(innerCompilationCount).toBe(1);
expect(element.text()).toBe('BazFooBar');
});
});

it('should lazily compile the contents of directives that are transclude element', function() {
var innerCompilationCount = 0, transclude;

module(function() {
directive('trans', valueFn({
transclude: 'element',
controller: function($transclude) {
transclude = $transclude;
}
}));

directive('inner', valueFn({
template: '<span>FooBar</span>',
compile: function() {
innerCompilationCount +=1;
}
}));
});

inject(function($compile, $rootScope) {
element = $compile('<div><trans><inner></inner></trans></div>')($rootScope);
expect(innerCompilationCount).toBe(0);
transclude(function(child) { element.append(child); });
expect(innerCompilationCount).toBe(1);
expect(element.text()).toBe('FooBar');
});
});

it('should lazily compile transcluded directives with ngIf on them', function() {
var innerCompilationCount = 0, outerCompilationCount = 0, transclude;

module(function() {
directive('outer', valueFn({
transclude: true,
compile: function() {
outerCompilationCount += 1;
},
controller: function($transclude) {
transclude = $transclude;
}
}));

directive('inner', valueFn({
template: '<span>FooBar</span>',
compile: function() {
innerCompilationCount +=1;
}
}));
});

inject(function($compile, $rootScope) {
$rootScope.shouldCompile = false;

element = $compile('<div><outer ng-if="shouldCompile"><inner></inner></outer></div>')($rootScope);
expect(outerCompilationCount).toBe(0);
expect(innerCompilationCount).toBe(0);
expect(transclude).toBeUndefined();
$rootScope.$apply('shouldCompile=true');
expect(outerCompilationCount).toBe(1);
expect(innerCompilationCount).toBe(0);
expect(transclude).toBeDefined();
transclude(function(child) { element.append(child); });
expect(outerCompilationCount).toBe(1);
expect(innerCompilationCount).toBe(1);
expect(element.text()).toBe('FooBar');
});
});

it('should eagerly compile multiple directives with transclusion and templateUrl/replace', function() {
var innerCompilationCount = 0;

module(function() {
directive('outer', valueFn({
transclude: true
}));

directive('outer', valueFn({
templateUrl: 'inner.html',
replace: true
}));

directive('inner', valueFn({
compile: function() {
innerCompilationCount +=1;
}
}));
});

inject(function($compile, $rootScope, $httpBackend) {
$httpBackend.expectGET('inner.html').respond('<inner></inner>');
element = $compile('<outer></outer>')($rootScope);
$httpBackend.flush();

expect(innerCompilationCount).toBe(1);
});
});
});


Expand Down