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

Support lazy inputFile.addJavaScript for substantial (re)build time savings. #9983

Merged
merged 19 commits into from Jun 13, 2018

Conversation

@benjamn
Member

benjamn commented Jun 12, 2018

In Meteor 1.7.0.1, multiple developers noticed that the less package takes longer than expected to compile .less files (#9957). There are a number of potential reasons for this deviation, but by far the most important reason is that Meteor compiler plugins in general (not just the less package) are expected to compile every file before Meteor has determined (in the ImportScanner) whether or not those files are actually used by the application.

The original reason for this architecture was that existing build plugins widely used by the community were accustomed to receiving a complete batch of all the files they needed to process, up front, all at once. This was especially important for compiler plugins that support importing other files of the same type, such as less and fourseven:scss. In Meteor 1.2, @glasser found a clever way to continue supporting that behavior, and we've stuck with that solution ever since.

By contrast, any system that attempts to scan the dependency graph of an application needs to be able to compile one file at a time. Most recently, I was reminded of this limitation when #9968 did not work, because my attempt to skip compiling .less files found in node_modules or imports had the unfortunate consequence of preventing them from being imported later by JavaScript modules using require or import.

For a long time, I thought these two approaches were irreconcilable. Either Meteor would have to maintain backwards compatibility despite the performance cost, or we would have to overhaul the compiler plugin system and deprecate existing plugins.

As this PR demonstrates, it turns out we can have it both ways. When calling inputFile.addJavaScript (or any other addWhatever method, such as addStylesheet), a compiler plugin may now provide an initial set of options (such as the path of the file) as well as a lazy finalizer function that can be called to finish compilation—but only if the file is actually used by the application.

Here's what that looks like in the babel-compiler package:

--- a/packages/babel-compiler/babel-compiler.js
+++ b/packages/babel-compiler/babel-compiler.js
@@ -21,15 +21,29 @@ var hasOwn = Object.prototype.hasOwnProperty;
 var isMeteorPre144 = semver.lt(process.version, "4.8.1");
 
 BCp.processFilesForTarget = function (inputFiles) {
+  var compiler = this;
+
   // Reset this cache for each batch processed.
   this._babelrcCache = null;
 
   inputFiles.forEach(function (inputFile) {
-    var toBeAdded = this.processOneFileForTarget(inputFile);
-    if (toBeAdded) {
-      inputFile.addJavaScript(toBeAdded);
+    if (inputFile.supportsLazyCompilation) {
+      inputFile.addJavaScript({
+        path: inputFile.getPathInPackage(),
+        hash: inputFile.getSourceHash(),
+        bare: !! inputFile.getFileOptions().bare
+      }, function () {
+        return compiler.processOneFileForTarget(inputFile);
+      });
+    } else {
+      var toBeAdded = compiler.processOneFileForTarget(inputFile);
+      if (toBeAdded) {
+        inputFile.addJavaScript(toBeAdded);
+      }
     }
-  }, this);
+  });
 };
 
 // Returns an object suitable for passing to inputFile.addJavaScript, or
 // null to indicate there was an error, and nothing should be added.
 BCp.processOneFileForTarget = function (inputFile, source) {

In other words, compiler plugins still receive a complete list of all files they should process, in case they depend on that behavior, but they have the option of postponing costly compilation until later.

Note that the compiler plugin system has the freedom to invoke the callback as soon as it likes (even immediately, as I initially implemented it, just to check my sanity), though in practice the ImportScanner is careful to avoid triggering the callback until it encounters the file in the dependency graph.

As a demonstration of the impact of this change, here's a fresh build of a new meteor create --full application without this optimization:

> meteor create --full eager-addJavaScript-test
> cd eager-addJavaScript-test
> METEOR_PROFILE=20 meteor
...
| Top leaves:
| Babel.compile...........................................25,564 ms (355)
...

and here's the same profiling output with this optimization enabled:

> meteor create --full lazy-addJavaScript-test
> cd lazy-addJavaScript-test
> METEOR_PROFILE=20 meteor
...
| Top leaves:
| Babel.compile...........................................10,110 ms (199)
...

If you do the math, the average time per file is approximately the same sometimes more, sometimes less; but the lazy version always wins because it compiles less than half as many files. 💇 💇‍♂️

If you're a compiler plugin maintainer (cc @sebakerckhof @GeoffreyBooth et al), please take note of this new API, and especially the inputFile.supportsLazyCompilation feature detection (so you don't have to stop supporting older versions of Meteor).

If you're an application developer, you won't see these benefits until you update to Meteor 1.7.1, but we hope this gives you an additional reason to help test the next 1.7.1 beta release.

benjamn added some commits Jun 9, 2018

Support lazy compilation of inputFile.add{JavaScript,Stylesheet,...} …
…resources.

One limitation of Meteor's current compiler plugins system is that every
file we *might* use must be compiled before we know whether it *will* be
used by the application (which is something we only find out when we later
run the `ImportScanner`). More specifically, when inputFile.addJavaScript
is called, any information relevant to the current file must already have
been computed, even if the file will never be used.

This commit begins the process of supporting a lazy version of the
`inputFile.addJavaScript` method. For consistency, this interface is
supported by other methods like `inputFile.addStylesheet`, though it may
not make as much sense for non-JavaScript resources.

In this very basic initial implementation, the `lazyFinalizer` function is
called immediately, so that subsequent code can keep pretending that all
relevant information was eagerly provided.

The next step will be waiting to call `lazyFinalizer` until the last
possible moment, so that we can skip a potentially huge amount of
unnecessary compilation time.
Let CachingCompiler subclasses implement compileOneFileLater.
If you're subclassing `CachingCompiler` or `MultiFileCachingCompiler`, you
can now implement a `compileOneFileLater` (emphasis on `Later`) to opt into
the new lazy compilation strategy.

If you implement this method, and `inputFile.supportsLazyCompilation` is
true, then the `addCompileResult` will not be called, though it is
probably a good idea to keep any existing `addCompileResult` methods, just
in case `inputFile.supportsLazyCompilation` is not truthy.

This will be an important part of a proper solution to the issues I
described (but failed to fix) in my broken PR #9968.
Bump minor version of less package to 2.8.0.
We should really update to the latest version of the less npm package
(3.0.4 at the time this commit message was written).
Stop checking isRoot before calling compileOneFileLater.
Now that compilation of compile-to-CSS files in imports/ and node_modules/
is actually lazy, we can safely call compileOneFileLater for all
inputFiles without worrying about accidental compilation.
data: result.css,
sourceMap: result.sourceMap,
};
});

This comment has been minimized.

@benjamn

benjamn Jun 12, 2018

Member

If you're already using CachingCompiler or MultiFileCachingCompiler to implement your compiler plugins, you should implement a new method called compileOneFileLater that works roughly like this.

).then(result => {
Object.assign(this._initialOptions, result);
this._finalizerPromise = null;
})).await();

This comment has been minimized.

@benjamn

benjamn Jun 12, 2018

Member

It's too bad this implementation has to use Fibers to await the result of the lazyFinalizer function, but that's an implementation detail we can revisit in future versions of Meteor without changing the API.

@@ -825,14 +818,11 @@ export default class ImportScanner {
// Set file.imported to a truthy value (either "dynamic" or true).
file.imported = forDynamicImport ? "dynamic" : true;
if (file.error) {
if (file.reportPendingErrors &&
file.reportPendingErrors() > 0) {

This comment has been minimized.

@benjamn

benjamn Jun 12, 2018

Member

This tends to be the first place the ImportScanner forces full compilation of file.

@@ -249,19 +249,6 @@ export default class ImportScanner {
// something plausible. #6411 #6383
const absPath = pathJoin(this.sourceRoot, file.sourcePath);
const dotExt = "." + file.type;
const dataString = file.data.toString("utf8");

This comment has been minimized.

@benjamn

benjamn Jun 12, 2018

Member

Accessing file.data here (in addInputFiles) would have forced every file to be fully compiled, regardless of whether it was scanned.

@benjamn benjamn merged commit e5e3580 into devel Jun 13, 2018

19 checks passed

CLA Author has signed the Meteor CLA.
Details
ci/circleci: Clean Up Your tests passed on CircleCI!
Details
ci/circleci: Docs Your tests passed on CircleCI!
Details
ci/circleci: Get Ready Your tests passed on CircleCI!
Details
ci/circleci: Isolated Tests Your tests passed on CircleCI!
Details
ci/circleci: Test Group 0 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 1 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 10 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 2 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 3 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 4 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 5 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 6 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 7 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 8 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 9 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@pagesrichie

This comment has been minimized.

pagesrichie commented Jun 13, 2018

@benjamn Hi Ben, thanks for this pull - due to the way meteor compiles LESS, my semantic ui meteor (https://atmospherejs.com/semantic/ui, https://github.com/Semantic-Org/Semantic-UI-Meteor) LESS files takes 8-10 seconds upon every time I reload a page - even when I change a single line of text in my app completely unrelated to the css itself! I've realized its not a fault with the semantic ui package itself but with the way meteor compiles/reads all the LESS files upon every change.

I came across this post and read it - a little confused on a few parts -

  1. Has this fix already been finished or is this something still in the works?
  2. Will packages that use or have a bunch of LESS files need to incorporate different code in their packages to have this working or will it work automatically once this issue is fixed?
  3. The METEOR@1.7.1-beta.0 release that came out 8 days ago or the METEOR@1.7.0.2 release that came out a few hours ago - has the issue been resolved in there? If not, then when can be an expected time or release number where we can test if the issue has been resolved?

Thanks for looking into this important issue and looking forward to a response.

@benjamn

This comment has been minimized.

Member

benjamn commented Jun 13, 2018

@pagesrichie These changes will be available starting in 1.7.1-beta.1, which should be published in the next half hour or so.

As long as packages and application code are compiling .less files with a compiler plugin that leverages the new lazy inputFile.addStylesheet style (like the core less package), the packages/application will not have to do anything special to benefit from these changes.

In general, we merge PRs into devel (so if you're submitting a PR you should branch off of devel), and then periodically we merge devel into the current release-x.y.z branch, which happens to be release-1.7.1 right now. When the release is ready, we merge the release-x.y.z branch back into master, and then merge master into devel.

Simple! 💫🕸🍰😭

@pagesrichie

This comment has been minimized.

pagesrichie commented Jun 13, 2018

I just saw the METEOR@1.7.1-beta.1 release on the https://github.com/meteor/meteor/releases page. I will go ahead and update my meteor and hopefully it fixes the issue with https://github.com/Semantic-Org/Semantic-UI-Meteor files compilation!

And @benjamn yes that package is dependent on the meteor core less package. So I will try it out with this new meteor release and let you know.

@pagesrichie

This comment has been minimized.

pagesrichie commented Jun 14, 2018

@benjamn Not sure where to post this.. but there's an issue with the 1.7.10beta.2 release, getting an error here when the less package is enabled when I have my less files in the client dir, i even disabled semantic-ui package and if it has any less files to parse it crashes. (To re-enact, I added all my semantic-ui client files somewhere inside the client dir and then then simply enable less package or the less@2.8.0-beta171.2 package (that automatically got upgraded to when upgrading to this meteor release, and it just crashes with the following error):

/.meteor/packages/static-html/.1.2.2.wp43cd.99etr++os+web.browser+web.cordova/plugin.compileStaticHtmlBatch.os/npm/node_modules/meteor/promise/node_modules/meteor-promise/promise_server.js:190
throw error;
^

TypeError: First argument must be a string, Buffer, ArrayBuffer, Array, or array-like object.
at Function.Buffer.from (buffer.js:183:11)
at CssOutputResource.get (/tools/isobuild/compiler-plugin.js:923:23)
at CssOutputResource.get data [as data] (/tools/isobuild/compiler-plugin.js:901:28)
at resources.forEach.resource (/tools/isobuild/bundler.js:1164:28)
at Array.forEach ()
at sourceBatches.forEach.sourceBatch (/tools/isobuild/bundler.js:1137:17)
at Array.forEach ()
at ClientTarget.emitResources (/tools/isobuild/bundler.js:1072:19)
at ClientTarget.profileWrapper (/tools/tool-env/profile.js:283:16)
at buildmessage.enterJob (/tools/isobuild/bundler.js:837:12)
at /tools/utils/buildmessage.js:359:18
at exports.EnvironmentVariable.withValue (/tools/utils/fiber-helpers.js:89:14)
at /tools/utils/buildmessage.js:352:34
at exports.EnvironmentVariable.withValue (/tools/utils/fiber-helpers.js:89:14)
at /tools/utils/buildmessage.js:350:23
at exports.EnvironmentVariable.withValue (/tools/utils/fiber-helpers.js:89:14)
at Object.enterJob (/tools/utils/buildmessage.js:324:26)
at ClientTarget.make (/tools/isobuild/bundler.js:828:18)
at ClientTarget.profileWrapper (/tools/tool-env/profile.js:283:16)
at /tools/isobuild/bundler.js:3019:14
at profileWrapper (/tools/tool-env/profile.js:283:16)
at /tools/isobuild/bundler.js:3108:20
at Array.forEach ()
at Function.
.each.
.forEach (/Users/macpc/.meteor/packages/meteor-tool/.1.7.1-beta.2.1k5yg1e.6mxx++os.osx.x86_64+web.browser+web.browser.legacy+web.cordova/mt-os.osx.x86_64/dev_bundle/lib/node_modules/underscore/underscore.js:79:11)
at /tools/isobuild/bundler.js:3107:7
at /tools/utils/buildmessage.js:271:13
at exports.EnvironmentVariable.withValue (/tools/utils/fiber-helpers.js:89:14)
at /tools/utils/buildmessage.js:264:29
at exports.EnvironmentVariable.withValue (/tools/utils/fiber-helpers.js:89:14)
at /tools/utils/buildmessage.js:262:18
at exports.EnvironmentVariable.withValue (/tools/utils/fiber-helpers.js:89:14)
at /tools/utils/buildmessage.js:253:23
at exports.EnvironmentVariable.withValue (/tools/utils/fiber-helpers.js:89:14)
at Object.capture (/tools/utils/buildmessage.js:252:19)
at bundle (/tools/isobuild/bundler.js:3000:31)
at files.withCache (/tools/isobuild/bundler.js:2947:32)
at Object. (/tools/fs/files.js:1710:12)
at Object.profileWrapper [as withCache] (/tools/tool-env/profile.js:283:16)
at Object.exports.bundle (/tools/isobuild/bundler.js:2947:16)
at Profile.run (/tools/runners/run-app.js:579:36)
at profileWrapper (/tools/tool-env/profile.js:283:16)
at time (/tools/tool-env/profile.js:305:10)
at Function.run (/tools/tool-env/profile.js:501:14)
at bundleApp (/tools/runners/run-app.js:578:34)
at AppRunner._runOnce (/tools/runners/run-app.js:622:35)
at AppRunner._fiber (/tools/runners/run-app.js:880:28)
at /tools/runners/run-app.js:408:12
npm ERR! code ELIFECYCLE
npm ERR! errno 1

@abernix

This comment has been minimized.

Member

abernix commented Jun 14, 2018

Not sure where to post this..

@pagesrichie In general, the best place to post it would be in a new issue. Though you're right in your identification of this issue as a suspect.

The problem stems from this code:

let { data } = this._initialOptions;
if (! Buffer.isBuffer(data)) {
data = Buffer.from(data, "utf8");
}

...and a result of data being undefined (which is, of course, not a Buffer), thus trying to allocate a Buffer using Buffer.from on an undefined value — certain failure.

I suspect that data is undefined because compilation of (some?) particular .less file is resulting in an undefined css property here:

const result = await getResult();
return result && {
data: result.css,
sourceMap: result.sourceMap,
};

...possibly because of a compilation failure or potentially just because of a completely empty file (I'm not sure what less does here).

We should probably guard against this within this _get method, since data is supplied (or in this case, un-supplied) through the compiler plugin API. It's possible that it should be prevented earlier in the workflow (e.g. when the compiler plugin is invoked), but that seems like it might be a breaking change for some compiler plugin packages that we could avoid.

I'll defer to @benjamn since this is fresh on his mind.

@pagesrichie

This comment has been minimized.

pagesrichie commented Jun 14, 2018

Thanks @abernix for bringing this attention to @benjamn.

@benjamn plz inform when you've fixed this in a release, I am super looking forward to testing out quicker less compilations from semantic ui. Thanks Ben!

@benjamn

This comment has been minimized.

Member

benjamn commented Jun 15, 2018

@pagesrichie @abernix That problem should be fixed by #9998. In short, we were applying the CSS source map to a JS module that dynamically appended the CSS to the <head>, which caused the source map processing to get confused, because the JS lines don't have any meaningful relationship to the CSS text.

benjamn added a commit that referenced this pull request Jun 16, 2018

Be more defensive about lazy stylesheets that fail to compile.
I should have paid more attention to @abernix's analysis here, as it was
exactly right: #9983 (comment)

cc @pagesrichie

coagmano added a commit to coagmano/meteor-stylus that referenced this pull request Oct 10, 2018

coagmano added a commit to coagmano/meteor-stylus that referenced this pull request Oct 10, 2018

coagmano added a commit to coagmano/meteor-stylus that referenced this pull request Oct 10, 2018

coagmano added a commit to coagmano/meteor-stylus that referenced this pull request Oct 10, 2018

@benjamn benjamn deleted the inputFile.addJavaScript-lazyFinalizer-thunk branch Oct 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment