Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX lts] Avoid excessively calling Glimmer AST transforms. #16241

Merged
merged 1 commit into from
Feb 13, 2018

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Feb 13, 2018

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.

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.
@rwjblue rwjblue merged commit 8bd2be0 into emberjs:master Feb 13, 2018
@mmun mmun deleted the be-less-stupid branch February 14, 2018 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants