Skip to content

Commit

Permalink
[BUGFIX lts] Avoid excessively calling Glimmer AST transforms.
Browse files Browse the repository at this point in the history
During the Ember 2.15 cycle glimmer-vm was updated and contained a new
format for AST plugins.  This required that all "legacy" plugins be
wrapped (to avoid breakage of this intimate API). When the wrapping code
was implemented two distinct bugs were introduced that would have caused
custom AST transforms to be called many more times than in Ember < 2.15.

Bug #1: Accruing AST Transforms via re-registration
---------------------------------------------------

ember-cli-htmlbars allows addons to register AST plugins via the normal
ember-cli preprocessor registry system. When this support was added it
was not possible to provide plugins during a specific compilation, and
therefore all plugins are registered globally (even by current
ember-cli-htmlbars versions).

Due to the mechanism that ember-cli uses to build up the list of addons
and dependencies for use at build time ember-cli-htmlbars ends up
requiring the template compiler many times and subsequently registering
any AST transform plugins that are present using the exported
`registerPlugin` API. Since this tracks plugins in module state it is
possible to have `registerPlugin` called many times with the same
plugin. ember-cli-htmlbars attempts to mitigate the polution of plugins
by forcing the require.cache of the template compiler module to be
flushed. Unfortuneately, there are circumstances where this cache
invalidation doesn't work and we therefore grow the number of registered
AST plugins _very_ quickly (once for each nested addon / engine / app
that is in the project).

During the 2.15 glimmer-vm upgrade the previous deduping logic was
removed (due to the fact that "legacy" AST transforms were always
generating unique plugin instances). When combined with situations where
ember-cli-htmlbars was not properly clearing the require cache the
resulting build times of complex applications using AST plugins grew >
double.

Bug #2: Legacy AST Transform Compat Bug
---------------------------------------------------

In order to avoid breakage in addons leveraging the intimate AST plugin
APIs a wrapper layer was added. Unfortunately, the wrapper layer
assumed that there would only be a single `Program` node. However, in
glimmers AST every `BlockStatement` has a `Program` node (and _may_
have an inverse with another `Program` node). This mistake meant that
the legacy "wrapped" AST transforms would be instantiated and ran _many_
times per-template which generates quite a bit of wasted work.

Fixes Included
--------------

* Bring back the deduplication code ensuring that a given AST plugin is
only registered once.
* Fix the legacy AST transform wrapper code to _only_ instantiate and
invoke the legacy AST transform once for the outermost `Program` node.

(cherry picked from commit a95e314)
  • Loading branch information
rwjblue committed Feb 14, 2018
1 parent 5227ede commit 979e805
Show file tree
Hide file tree
Showing 2 changed files with 115 additions and 18 deletions.
50 changes: 35 additions & 15 deletions packages/ember-template-compiler/lib/system/compile-options.js
Expand Up @@ -16,48 +16,68 @@ export default function compileOptions(_options) {
options.plugins = { ast: [...USER_PLUGINS, ...PLUGINS] };
} else {
let potententialPugins = [...USER_PLUGINS, ...PLUGINS];
let providedPlugins = options.plugins.ast.map(plugin => wrapLegacyPluginIfNeeded(plugin));
let pluginsToAdd = potententialPugins.filter((plugin) => {
return options.plugins.ast.indexOf(plugin) === -1;
});
options.plugins.ast = options.plugins.ast.slice().concat(pluginsToAdd);
options.plugins.ast = providedPlugins.concat(pluginsToAdd);
}

return options;
}

export function registerPlugin(type, _plugin) {
if (type !== 'ast') {
throw new Error(`Attempting to register ${_plugin} as "${type}" which is not a valid Glimmer plugin type.`);
}

let plugin;
function wrapLegacyPluginIfNeeded(_plugin) {
let plugin = _plugin;
if (_plugin.prototype && _plugin.prototype.transform) {
plugin = (env) => {
let pluginInstantiated = false;

return {
name: _plugin.constructor && _plugin.constructor.name,

visitors: {
Program(node) {
let plugin = new _plugin(env);
if (!pluginInstantiated) {

pluginInstantiated = true;
let plugin = new _plugin(env);

plugin.syntax = env.syntax;
plugin.syntax = env.syntax;

return plugin.transform(node);
return plugin.transform(node);
}
}
}
};
};
};
} else {
plugin = _plugin;

plugin.__raw = _plugin;
}

return plugin;
}

export function registerPlugin(type, _plugin) {
if (type !== 'ast') {
throw new Error(`Attempting to register ${_plugin} as "${type}" which is not a valid Glimmer plugin type.`);
}

for (let i = 0; i < USER_PLUGINS.length; i++) {
let PLUGIN = USER_PLUGINS[i];
if (PLUGIN === _plugin || PLUGIN.__raw === _plugin) {
return;
}
}

let plugin = wrapLegacyPluginIfNeeded(_plugin);

USER_PLUGINS = [plugin, ...USER_PLUGINS];
}

export function removePlugin(type, PluginClass) {
export function unregisterPlugin(type, PluginClass) {
if (type !== 'ast') {
throw new Error(`Attempting to unregister ${PluginClass} as "${type}" which is not a valid Glimmer plugin type.`);
}

USER_PLUGINS = USER_PLUGINS.filter((plugin) => plugin !== PluginClass);
USER_PLUGINS = USER_PLUGINS.filter((plugin) => plugin !== PluginClass && plugin.__raw !== PluginClass);
}
@@ -1,6 +1,5 @@
import { compileOptions } from '../../index';
import { defaultPlugins } from '../../index';
import { moduleFor, AbstractTestCase } from 'internal-test-helpers';
import { compile, compileOptions, defaultPlugins, registerPlugin, unregisterPlugin } from '../../index';
import { moduleFor, AbstractTestCase, RenderingTestCase } from 'internal-test-helpers';

moduleFor('ember-template-compiler: default compile options', class extends AbstractTestCase {
['@test default options are a new copy'](assert) {
Expand All @@ -18,3 +17,81 @@ moduleFor('ember-template-compiler: default compile options', class extends Abst
}
}
});

let customTransformCounter = 0;
class CustomTransform {
constructor(options) {
customTransformCounter++;
this.options = options;
this.syntax = null;
}

transform(ast) {
let walker = new this.syntax.Walker();

walker.visit(ast, node => {
if (node.type !== 'ElementNode') {
return;
}

for (var i = 0; i < node.attributes.length; i++) {
let attribute = node.attributes[i];

if (attribute.name === 'data-test') {
node.attributes.splice(i, 1);
}
}
});

return ast;
}
}

class CustomPluginsTests extends RenderingTestCase {
afterEach() {
customTransformCounter = 0;
return super.afterEach();
}

['@test custom plugins can be used']() {
this.render('<div data-test="foo" data-blah="derp" class="hahaha"></div>');
this.assertElement(this.firstChild, {
tagName: 'div',
attrs: { class: 'hahaha', 'data-blah': 'derp' },
content: ''
});
}

['@test wrapped plugins are only invoked once per template'](assert) {
this.render('<div>{{#if falsey}}nope{{/if}}</div>');
assert.equal(customTransformCounter, 1, 'transform should only be instantiated once');
}
}

moduleFor('ember-template-compiler: registerPlugin with a custom plugins', class extends CustomPluginsTests {
beforeEach() {
registerPlugin('ast', CustomTransform);
}

afterEach() {
unregisterPlugin('ast', CustomTransform);
return super.afterEach();
}

['@test custom registered plugins are deduplicated'](assert) {
registerPlugin('ast', CustomTransform);
this.registerTemplate('application', '<div data-test="foo" data-blah="derp" class="hahaha"></div>');
assert.equal(customTransformCounter, 1, 'transform should only be instantiated once');
}
});

moduleFor('ember-template-compiler: custom plugins passed to compile', class extends RenderingTestCase {
// override so that we can provide custom AST plugins to compile
compile(templateString) {
return compile(templateString, {
plugins: {
ast: [CustomTransform]
}
});
}
});

0 comments on commit 979e805

Please sign in to comment.