elidoran / cosmos-browserify

Browserify npm modules for client side in Meteor packages
MIT License
78 stars 12 forks source link

Caching of bundle for when app.browserify.js doesn't change #11

Closed optilude closed 9 years ago

optilude commented 9 years ago

Meteor app (re)builds can become very slow when browserify gets slow. For example, I included jszip and XLSX and client-only reloads now take up to 90 seconds!

I understand that browserify is slow sometimes, but in 99% of cases it's only my application code that changes and not anything require()d through app.browserify.js. Is there some way cosmos:browserify could avoid re-running browserify on each build if that particular file hasn't changed?

elidoran commented 9 years ago

Meteor controls when a build plugin is run. And, when the plugin runs it must provide a result to the CompileStep or its previous result will be removed from the running client. Too bad we can't tell the Plugin manager which file changes should trigger a build.

While looking at this I noticed it will run twice when the app.browserify.js file is outside the client folder; once for client and once for server? So, I recommend putting your app.browserify.js file inside the client folder. It's client-only stuff anyway. It only runs once when I moved it there.

I created a new branch, cache-results, with caching in it. It uses the cached version unless it was made before the modified time of the following files:

  1. the browserify file
  2. the browserify options file
  3. the npm-shrinkwrap.json file

I haven't written the tests for it yet. Once I do, then I'll publish it.

Also, these changes are coming in on top of upgrading browserify to version 10.2.4 (from 9.0.8).

Please check this out and let me know if it helps you so I can make changes, if necessary, before publishing.

Thank you

optilude commented 9 years ago

Awesome! Will try tonight.

It was in the client/ folder already, but a cache like the one you describe sounds correct.

I presume I just check it out into packages/ to run with the branch version?

Martin

— Sent from Mailbox

On Sun, Jul 5, 2015 at 8:21 AM, Eli Doran notifications@github.com wrote:

Meteor controls when a build plugin is run. And, when the plugin runs it must provide a result to the CompileStep or its previous result will be removed from the running client. Too bad we can't tell the Plugin manager which file changes should trigger a build. While looking at this I noticed it will run twice when the app.browserify.js file is outside the client folder; once for client and once for server? So, I recommend putting your app.browserify.js file inside the client folder. It's client-only stuff anyway. It only runs once when I moved it there. I created a new branch, cache-results, with caching in it. It uses the cached version unless it was made before the modified time of the following files:

  1. the browserify file
  2. the browserify options file
  3. the npm-shrinkwrap.json file I haven't written the tests for it yet. Once I do, then I'll publish it. Also, these changes are coming in on top of upgrading browserify to version 10.2.4 (from 9.0.8). Please check this out and let me know if it helps you so I can make changes, if necessary, before publishing.

Thank you

Reply to this email directly or view it on GitHub: https://github.com/elidoran/cosmos-browserify/issues/11#issuecomment-118590670

optilude commented 9 years ago

Hi,

I've just tried the branch, but unfortunately the cache doesn't appear to be working helping.

These are the packages I have installed:

meteor-platform
accounts-password
aldeed:collection2
twbs:bootstrap
sanjo:jasmine
velocity:html-reporter
mrt:accounts-admin-ui-bootstrap-3
alanning:roles
cosmos:browserify
meteorhacks:npm

npm-container
react

I have a top-level packages.json with:

{
    "classnames": "2.1.2",
    "react-dnd": "1.1.3",
    "moment": "2.10.3",
    "react": "0.13.3",
    "react-bootstrap": "0.23.5",
    "bootstrap-select": "1.7.2",
    "react-router": "0.13.3",
    "react-router-bootstrap": "0.16.0",
    "exposify": "0.4.3",
    "d3": "3.5.5",
    "nvd3": "1.8.1",
    "underscore": "1.8.3",
    "bootbox": "4.4.0",
    "jszip": "2.5.0",
    "xlsx": "0.8.0"
}

I then have in client/lib/app.browserify.js:

/* global require, classNames:true, ReactDnD:true, moment:true, bootbox:true,
    Router:true, ReactBootstrap:true, ReactRouterBootstrap:true,
    d3:true, nv:true, _:true, JSZip: true, XLSX:true
*/

_ = require('underscore'); // upgrade
moment = require('moment');
classNames = require('classnames');

require('bootstrap-select'); // injects itself into jQuery
bootbox = require('bootbox');

Router = require('react-router');
ReactBootstrap = require('react-bootstrap');
ReactRouterBootstrap = require('react-router-bootstrap');

// ReactBootstrap.Select = require('react-bootstrap-select');

ReactDnD = require('react-dnd');
ReactDnD.HTML5 = require('react-dnd/modules/backends/HTML5');

d3 = require('d3');
nv = require('nvd3');

JSZip = require('jszip');
XLSX = require('xlsx');

The last two (JSZip and XLSX) in particular are a killer, I only really noticed the slowdown when I added them.

I also have client/lib/app.browserify.options.json:

{
    "transforms": {
        "exposify": {
            "global": true,
            "expose": {
                "jquery": "jQuery",
                "react": "React",
                "react/addons": "React"
            }
        }
    }
}

I can see app.browserify.js.cached being created as well as app.browserify.js.map, both in client/lib next to app.browserify.js.

I then make a trivial change in one of the .jsx files in client/ that contains my application, it takes 1.5 minutes until I get the Client modified -- refreshing message and the browser reloads.

optilude commented 9 years ago

Interestingly, I put some debug statements in the plugin code. It seems to me that checkFileChanges is returning false and getResult is using the cached file. However, the build is still very very slow with those two modules in (and gets slower the more stuff I put in app.browserify.js.)

It looks like all of processFile is pretty fast with the cache on (so the cache definitely helps). The string returned from getResults has a length of "2,659,369" characters, which is obviously huge. I don't know what more Meteor does with this once it's returned that is making it take so long afterwards. :(

The build is not slow if I take single-file dist versions of jszip and xlsx and put them in client/lib, so it has something to do with the build pipeline.

Oh, and the cache definitely helps. It shaves about 10 seconds off. But unfortunately the downstream problem is much bigger...

optilude commented 9 years ago

I haven't quite given up on trying to figure out why this is so slow in 'app' mode, but as a workaround, if I put all my dependencies in a package, then changes to my app code doesn't trigger a rebuild.

To do this, I ran meteor create --package app-deps, and then meteor add app-deps

In packages/app-deps, I created client.browserify.js and client.browserify.options.json with the same content as the app.browserify.js and app.browserify.options.json examples above.

I then edited packages/app-deps/package.js to:

/* global Package, Npm */
"use strict";

Package.describe({
    name: 'app-deps',
    version: '0.0.1',
    summary: 'Private package that loads NPM dependencies for the app',
    git: '',
    documentation: null
});

Npm.depends({
    "classnames": "2.1.2",
    "react-dnd": "1.1.3",
    "moment": "2.10.3",
    "react": "0.13.3",
    "react-bootstrap": "0.23.5",
    "bootstrap-select": "1.7.2",
    "react-router": "0.13.3",
    "react-router-bootstrap": "0.16.0",
    "exposify": "0.4.3",
    "d3": "3.5.5",
    "nvd3": "1.8.1",
    "underscore": "1.8.3",
    "bootbox": "4.4.0",
    "jszip": "2.5.0",
    "xlsx": "0.8.0"
});

Package.onUse(function(api) {
    api.versionsFrom('1.1.0.2');
    api.use([
        'cosmos:browserify@0.5.0',
        'react@0.1.0',
    ], 'client');

    api.addFiles([
        'client.browserify.js',
        'client.browserify.options.json'
    ], 'client');

    api.export('_', 'client');
    api.export('moment', 'client');
    api.export('classNames', 'client');
    api.export('bootbox', 'client');
    api.export('Router', 'client');
    api.export('ReactBootstrap', 'client');
    api.export('ReactRouterBootstrap', 'client');
    api.export('ReactDnD', 'client');
    api.export('d3', 'client');
    api.export('nv', 'client');
    api.export('JSZip', 'client');
    api.export('XLSX', 'client');
});

What's happening here is:

The DRY-violation when using api.export is kind of annoying, but the good news it that at least now my app reloads are almost instantaneous again.

If I modify anything in packages/app-deps, then that reload is very slow again.

This still leaves a few questions:

elidoran commented 9 years ago

Well, the cache works, that's something at least :)

Perhaps the slowness is because Meteor's build system uses synchronous IO instead of asynchronous streaming IO? I have to pass the entirety of both the browserify result and its source map as strings to CompileStep's addJavaScript function. That makes the build process a bit of a memory hog. They also provide the original source as a Buffer. So, that's three files completely in memory at once. It'd be nice if the original source was provided as a readable stream and we could pipe it through our transforms back to the build process. I could still provide my cached files as readable streams, too.

api.export() is necessary to tell Meteor to provide the variables from package scope to app scope. Having the package's variables encapsulated by default is a good thing. So, require() is pulling them into the package scope, and api.export() is sharing them in app scope.

That said, when exporting many variables there's extra typing involved. You can combine all those api.export() calls into one by providing the names in an array as the first argument.

api.export([
  '_', 
  'moment', 
  'classNames'//, 
  ///...
], 'client');

You still have to maintain them in both files though. So, you could assign all those things into a single object in your browserify file and then export that object. Give it a helpful name, or name it something generic like 'App'. Then, when you add more stuff you only need to alter your browserify file.

// in your browserify.js file:
Util = {
  d3: require('d3'),
  nv: require('nvd3'),
  JSZip: require('jszip'),
  XLSX: require('xlsx')
};

// and in package.js do
api.export('Util', 'client');

With the cache working I'll make the tests and then publish it.

Thank you

optilude commented 9 years ago

Hi,

That's very much what I'm doing. I have a Dependencies object and then I'm exporting that. :)

I doubt the slowness is caused by the amount of memory used, it's still only a few Mb. Something must be trying to parse/do something with the file contents that requires processing. Quite annoying.

In the meantime, the package option works pretty well.

+1 to publishing the caching stuff. Maybe you want a switch to turn it on/off?

elidoran commented 9 years ago

I'm glad using a package is helping; that's something at least. Also, I prefer working with packages, so, perhaps you'll get some other advantages.

You're right, I'm sure it does a bunch of processing too. I get annoyed with the memory hogging tho. It may be holding all the build steps in memory as it goes. When I am using a small VM for dev work Meteor crashes when it uses up all the memory (half gig). I'd prefer they switched to streaming IO.

Anyway, adding a switch to turn off caching is a good idea. I added a cache option to the options file so it can be turned off.

I finally tweaked the tests enough so they are working with the new changes and published 0.5.0.

Let me know if you have any other feedback for this feature and I'll see what I can do. Thank you.

elidoran commented 9 years ago

@optilude I made a new branch, generate, which is a different style of caching. Instead, it creates a .js file using suffix .gen.js. Then, Meteor handles that like it does any other JS file. And, this plugin doesn't do anything when it sees that file doesn't need to be remade.

When the plugin generates the file anew it triggers a second build to process the changed .gen.js file, but, the plugin quickly realizes there's nothing for it to do. And, from then on, changes to the app don't require Meteor to reprocess the generated file or require the plugin to do work.

This way, Meteor doesn't need to do any work for the browserified result file unless it changes. My testing shows this way eliminates that weird time Meteor spends processing the cached file in 0.5.0.

I haven't documented it yet. Basically, instead of using an option cache:true to control caching, there is now a mode option which accepts: gen, cache, and redo. By default it does the new way, mode gen. Mode cache is the same as I introduced in 0.5.0 (cache:true). Mode redo is what it did before 0.5.0: rebrowserified the result every time; a redo (also cache:false in 0.5.0).

Please try out this new branch and see if it speeds up the build for you. If so, I'll clean it up and publish a 0.6.0 release.

Thank you.

elidoran commented 9 years ago

Also, I should mention using mode gen to make a new file doesn't work in a package because we must specify the name of each file to include. I haven't dealt with that yet. That'll be part of the cleanup I'll do once I'm certain this actually helps. I just thought of doing it this way to speed up using it at the app level and threw it together in a separate branch to get some feedback.

elidoran commented 9 years ago

Unless someone wants to run this plugin with a version of Meteor before 1.2 this branch is unnecessary. It provides a considerable speed increase above the caching I was doing in 0.5.0, however, so does Meteor's 1.2 Build API.

So, I'm closing this issue. Use cosmos:browserify 0.7.0+