add delayed transpilation by kellyselden · Pull Request #7501 · ember-cli/ember-cli

kellyselden · 2017-12-13T03:02:25Z

I would like some feedback. Things to note:

we have to detect files that are already AMD because certain addons do their own transpilation.
we have to detect addons in legacy 'modules/' dir and undo to transpile correctly.
removed treeForAddon/compileModules warning, because it is now the desired behavior.
removed compileTemplates no template processor warning, because it is now the desired behavior.

Sample console output:

DEPRECATION: Addon "@html-next/vertical-collection" (found at "C:\Users\kelly\Desktop\my-app\node_modules\@html-next\vertical-collection") is manually placing files in the legacy "modules" folder. Support for this will be removed in a future version.
DEPRECATION: Addon "ember-data" (found at "C:\Users\kelly\Desktop\my-app\node_modules\ember-data") is manually placing files in the legacy "modules" folder. Support for this will be removed in a future version.

Left to do:

get fork of ember-cli-babel merged add a no-op optimization emberjs/ember-cli-babel#204
get Addon._treeFor optimization #7634 merged
disable for babel 5
put behind feature flag

rtablada · 2017-12-27T19:55:35Z

@kellyselden should the AMD Excluder be extracted into broccoli-funnel-amd-excluder? I'm not sure if that could potentially be helpful as a separate package rather than as part of Ember CLI?

Though I know that adds a maintenance cost too.

rtablada · 2017-12-27T19:58:07Z

lib/broccoli/amd-excluder.js

+      let source = fs.readFileSync(inputFilePath, 'utf8');
+
+      // ember-data and others compile their own source
+      if (source.indexOf('define(') === 0) {


Should be fine, but I'm not 100 about potential edgecases. It would likely not be compatible JS anyway so this should be good enough to figure out if something is AMD.

On the other hand, what if the module is exported for AMD but doesn't define immediately.

There are a lot of potential edge cases, but in reality we only have to worry about how babel formats the AMD code, which this should cover completely?

kellyselden · 2017-12-27T19:59:27Z

@rtablada I am in favor of extracting it.

rtablada · 2017-12-27T20:00:11Z

I'd like to see what documentation (if any) is needed for addon authors to make sure their addons work with this.

Bonus points for docs on how to utilize these changes in addons.

kellyselden · 2017-12-27T20:01:23Z

@rtablada The only thing addons have to worry about is the first bullet point

addons no longer receive preprocessTree and postprocessTree hooks.

Turbo87 · 2017-12-28T13:30:33Z

lib/broccoli/amd-excluder.js

+const Funnel = require('broccoli-funnel');
+const walkSync = require('walk-sync');
+
+class AmdExcluder extends Funnel {


can you use https://github.com/broccolijs/broccoli-test-helper to write a few tests for this class?

a little bit of documentation on this class might also be nice. e.g. answering the question that @rtablada raised about this being specialized to detect AMD modules compiled by Babel (5? 6? 7?)

Turbo87 · 2017-12-28T13:32:35Z

lib/broccoli/amd-excluder.js

+    let files = walkSync(inputPath, {
+      directories: false,
+      globs: [`**/*.js`],
+    });


does build() have to be sync? otherwise it might make sense to read and process several files in parallel 🤔

Doesn't have to be. Which package should I use for async walking?

I guess walkSync is okay, but the loop afterwards could possibly be concurrent

Changed to async readFile. Was this what you were thinking?

Turbo87 · 2017-12-28T13:35:11Z

lib/broccoli/amd-excluder.js

+
+      // ember-data and others compile their own source
+      if (source.indexOf('define(') === 0) {
+        this.exclude.push(file);


will this fill up this.exclude with duplicate entries on rebuilds? we might have to reset this.exclude = []; at the beginning of build() 🤔

Turbo87 · 2017-12-28T13:39:31Z

lib/broccoli/ember-app.js

+    precompiledSource = preprocessJs(precompiledSource, '/', '/', {
+      annotation: `_compileAddonJs`,
+      registry: this.registry,
+    });


I'm wondering if in terms of performance it would be better to teach preprocessJs() about the AMD exclusion rule 🤔 but then again that is something that can probably be refactored later...

kellyselden · 2018-01-27T21:13:50Z

wooo tests finally green.

twokul

Reviewed with @rwjblue (and @kellyselden / @twokul), this seems good with a couple of minor cleanup points...

twokul · 2018-01-29T22:35:25Z

tests/unit/models/addon-test.js

-        `(NOT \`devDependencies\`) in \`${addon.name}\`'s \`package.json\`.`
-      );
-    });
+    // it('should throw a useful error if a template compiler is not present -- non-pods', function() {


needs to be deleted

twokul · 2018-01-29T22:35:34Z

lib/models/addon.js

-          `Please make sure your template precompiler (commonly \`ember-cli-htmlbars\`) is listed in \`dependencies\` ` +
-          `(NOT \`devDependencies\`) in \`${this.name}\`'s \`package.json\`.`);
-      }
+      // if (!registryHasPreprocessor(this.registry, 'template')) {


needs to be deleted

twokul · 2018-01-29T22:37:59Z

lib/broccoli/ember-app.js

      });

-      this._cachedAddonTree = new Funnel(combinedAddonTree, {
+      let compiledAddonTree = this._compileAddonTree(combinedAddonTree);


Can we add some broccoli-debug trees for before and after? I'm mostly thinking this will be super useful for seeing where some transpilation issues might be coming from...

I emulated the style from the assembler. @twokul you may want to verify.

twokul · 2018-01-29T22:38:21Z

lib/broccoli/ember-app.js

  }

+  _compileAddonTemplates(tree) {
+    if (registryHasPreprocessor(this.registry, 'template')) {


Can we actually get here?

I remember why I added it. Without it we get errors in the styles tests https://travis-ci.org/ember-cli/ember-cli/jobs/337422087#L805-L813 missing template preprocessor. I will add more mocking to the styles tests.

twokul · 2018-01-29T22:43:56Z

lib/broccoli/amd-excluder.js

+      let inputFilePath = path.join(inputPath, file);
+      return fs.readFile(inputFilePath, 'utf8').then(source => {
+        if (source.indexOf('define(') === 0) {
+          this.exclude.push(file);


Lets add a warning here...

this.exclude needs to be reset here (and we probably need to clear whatever funnel caches have been build up)

exclude is reset and it prints to console. the code has been moved to https://github.com/ember-cli/broccoli-amd-funnel. I also added tests.

twokul · 2018-01-29T22:47:36Z

lib/broccoli/ember-app.js

+  }
+
+  _compileAddonJs(tree) {
+    let precompiledSource = new AmdExcluder(tree, {


lets add a flag to opt-in or out of this tree as an option to new EmberApp, the default value should be true (e.g. use the amd excluder) but ultimately we should try to change it to false once a good number of popular addons have been updated...

twokul · 2018-01-29T22:53:37Z

lib/models/addon.js

+    // treeForAddon without calling super
+    // they need the original params preserved because they call
+    // babel themselves and expect compilation the old way
+    this.options[emberCLIBabelConfigKey] = Object.assign({}, original, {


This really should be moved back to the constructor if at all possible, setting it here means that others that depend on it earlier just don't have access to the values when they expect...

I remember why I placed it here instead of the constructor. Having it in the constructor means I need to handle the legacy /modules using broccoli-module-normalizer. Making that change now.

twokul · 2018-02-18T00:35:40Z

so far, both @kellyselden and I, are seeing anywhere between 15-30% slowdown (depending on the application we run benchmarks against).

I ran tests against travis-web.

here's what a flame chart w/o delayed transpilation looks like:

and here's a chart w/ delayed transpilation:

Note, that we started running Babel twice (essentially unnecessary work). also, order in which we process add-ons have changed (from active-model-adapter, ember-cli-mirage, ember-data, ember-modal-dialog, ember-moment to ember-data, ember-cli-mirage, ember-moment). A thing to note is that some of the add-ons "disappeared" from flame graph and a wide gap on "unaccounted" work appeared. It's either ember-cli-inline-images or GC kicks in.

We can optimize and don't run Babel twice but there's a gap we're still seeing:

kellyselden · 2018-02-25T03:14:45Z

All this new code is guarded around the feature flag DELAYED_TRANSPILATION. Because of this, and because @rwjblue and @twokul have reviewed this multiple times with no remaining problems, I think it is safe to merge.

stefanpenner · 2018-04-04T16:10:48Z

I believe this PR is breaking things, specifically we seem to trying to transpile handlebars templates that exist <addon>/src/<somefolder>/templates/model.hbs

rwjblue · 2018-04-04T19:20:21Z

@stefanpenner - This should have been fixed by #7735, no?

kellyselden force-pushed the delay-transpilation-3 branch 2 times, most recently from 6db915c to 283f446 Compare December 21, 2017 05:15

rtablada reviewed Dec 27, 2017

View reviewed changes

Turbo87 reviewed Dec 28, 2017

View reviewed changes

kellyselden force-pushed the delay-transpilation-3 branch 7 times, most recently from 4f09907 to 529d6af Compare January 27, 2018 19:50

kellyselden force-pushed the delay-transpilation-3 branch 2 times, most recently from 614ab47 to 4670071 Compare January 28, 2018 00:34

kellyselden mentioned this pull request Jan 28, 2018

extract registryHasPreprocessor to util #7583

Merged

kellyselden force-pushed the delay-transpilation-3 branch 2 times, most recently from f7dc658 to d83a323 Compare January 28, 2018 01:30

kellyselden requested review from Turbo87, rwjblue and twokul January 28, 2018 03:37

kellyselden force-pushed the delay-transpilation-3 branch from d83a323 to 4132594 Compare January 28, 2018 04:31

twokul reviewed Jan 29, 2018

View reviewed changes

kellyselden force-pushed the delay-transpilation-3 branch 4 times, most recently from 5f3a5f6 to 3198d21 Compare February 5, 2018 00:28

kellyselden force-pushed the delay-transpilation-3 branch from 94063b2 to a2bb5e3 Compare February 18, 2018 05:05

kellyselden mentioned this pull request Feb 18, 2018

Addon._treeFor optimization #7634

Merged

kellyselden force-pushed the delay-transpilation-3 branch 4 times, most recently from 16af76f to 1113dc8 Compare February 21, 2018 18:38

kellyselden changed the title ~~WIP: add delayed transpilation~~ add delayed transpilation Feb 21, 2018

kellyselden force-pushed the delay-transpilation-3 branch 6 times, most recently from 0bee743 to 2da17aa Compare February 23, 2018 04:04

kellyselden mentioned this pull request Feb 23, 2018

extract _initDefaultBabelOptions #7643

Merged

kellyselden force-pushed the delay-transpilation-3 branch 4 times, most recently from 2cd5918 to 7801f39 Compare February 25, 2018 02:07

add delayed transpilation

0aec861

kellyselden force-pushed the delay-transpilation-3 branch from 7801f39 to 0aec861 Compare February 25, 2018 02:14

kellyselden merged commit 3a78ec3 into ember-cli:master Feb 25, 2018

kellyselden deleted the delay-transpilation-3 branch February 25, 2018 03:15

This was referenced Feb 25, 2018

only use the standard compilers to compile addon code #7649

Merged

compile all addons at once optimization #7650

Merged

rwjblue mentioned this pull request Mar 1, 2018

Module Unification Addons #7490

Merged

7 tasks

Turbo87 mentioned this pull request Mar 23, 2018

Ember 3.1 Release Blog Post emberjs/website#3230

Merged

13 tasks

Conversation

kellyselden commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rtablada commented Dec 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kellyselden commented Dec 27, 2017

Uh oh!

rtablada commented Dec 27, 2017

Uh oh!

kellyselden commented Dec 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kellyselden commented Jan 27, 2018

Uh oh!

twokul left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

twokul commented Feb 18, 2018

Uh oh!

kellyselden commented Feb 25, 2018

Uh oh!

stefanpenner commented Apr 4, 2018

kellyselden commented Dec 13, 2017 •

edited

Loading