Skip to content

Commit 96dfe08

Browse files
author
Robert Jackson
committed
[BUGFIX lts] Avoid excessively calling Glimmer AST transforms.
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 emberjs#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 emberjs#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)
1 parent 3625ba2 commit 96dfe08

File tree

2 files changed

+36
-15
lines changed

2 files changed

+36
-15
lines changed

packages/ember-template-compiler/lib/system/compile-options.js

+35-15
Original file line numberDiff line numberDiff line change
@@ -16,48 +16,68 @@ export default function compileOptions(_options) {
1616
options.plugins = { ast: [...USER_PLUGINS, ...PLUGINS] };
1717
} else {
1818
let potententialPugins = [...USER_PLUGINS, ...PLUGINS];
19+
let providedPlugins = options.plugins.ast.map(plugin => wrapLegacyPluginIfNeeded(plugin));
1920
let pluginsToAdd = potententialPugins.filter((plugin) => {
2021
return options.plugins.ast.indexOf(plugin) === -1;
2122
});
22-
options.plugins.ast = options.plugins.ast.slice().concat(pluginsToAdd);
23+
options.plugins.ast = providedPlugins.concat(pluginsToAdd);
2324
}
2425

2526
return options;
2627
}
2728

28-
export function registerPlugin(type, _plugin) {
29-
if (type !== 'ast') {
30-
throw new Error(`Attempting to register ${_plugin} as "${type}" which is not a valid Glimmer plugin type.`);
31-
}
32-
33-
let plugin;
29+
function wrapLegacyPluginIfNeeded(_plugin) {
30+
let plugin = _plugin;
3431
if (_plugin.prototype && _plugin.prototype.transform) {
3532
plugin = (env) => {
33+
let pluginInstantiated = false;
34+
3635
return {
3736
name: _plugin.constructor && _plugin.constructor.name,
3837

3938
visitors: {
4039
Program(node) {
41-
let plugin = new _plugin(env);
40+
if (!pluginInstantiated) {
41+
42+
pluginInstantiated = true;
43+
let plugin = new _plugin(env);
4244

43-
plugin.syntax = env.syntax;
45+
plugin.syntax = env.syntax;
4446

45-
return plugin.transform(node);
47+
return plugin.transform(node);
48+
}
4649
}
4750
}
48-
};
51+
};
4952
};
50-
} else {
51-
plugin = _plugin;
53+
54+
plugin.__raw = _plugin;
5255
}
5356

57+
return plugin;
58+
}
59+
60+
export function registerPlugin(type, _plugin) {
61+
if (type !== 'ast') {
62+
throw new Error(`Attempting to register ${_plugin} as "${type}" which is not a valid Glimmer plugin type.`);
63+
}
64+
65+
for (let i = 0; i < USER_PLUGINS.length; i++) {
66+
let PLUGIN = USER_PLUGINS[i];
67+
if (PLUGIN === _plugin || PLUGIN.__raw === _plugin) {
68+
return;
69+
}
70+
}
71+
72+
let plugin = wrapLegacyPluginIfNeeded(_plugin);
73+
5474
USER_PLUGINS = [plugin, ...USER_PLUGINS];
5575
}
5676

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

62-
USER_PLUGINS = USER_PLUGINS.filter((plugin) => plugin !== PluginClass);
82+
USER_PLUGINS = USER_PLUGINS.filter((plugin) => plugin !== PluginClass && plugin.__raw !== PluginClass);
6383
}

packages/ember-template-compiler/tests/system/compile_options_test.js

+1
Original file line numberDiff line numberDiff line change
@@ -17,3 +17,4 @@ QUnit.test('has default AST plugins', function(assert) {
1717
assert.ok(plugins.indexOf(plugin) > -1, `includes ${plugin}`);
1818
}
1919
});
20+

0 commit comments

Comments
 (0)