michaelficarra / commonjs-everywhere

:rainbow: minimal CommonJS browser bundler with aliasing, extensibility, and source maps
BSD 3-Clause "New" or "Revised" License
158 stars 21 forks source link

Incremental rebuilding time is too slow and takes O(n) time where n is the size of your project #63

Closed krisnye closed 11 years ago

krisnye commented 11 years ago

My project is very small right now. 50kb and depends on Sugar.js, so fully built output size is about 280kb.

It is taking about 4 seconds to rebuild on my Core 2 Duo 2.6 ghz machine.

I added some debug and investigated, and I found that almost all of the time is spent in the "escodegen.generate" function.

Your incremental building code seems plenty fast, but the escodegen is killing performance. The problem seems to be that we have to run the escodegen on ALL of the AST tree even though we only changed just a single line in one source file.

The bigger problem with this is that the performance will degrade further as the project grows. My project will grow from 50k source to probably about 1Meg of source, and the "incremental build" time will increase proportionally.

This makes the current solution unfit for a development environment, where the need to test in the browser requires you to have a very quick turn-around between editing, refreshing and testing. Personally, I require a build that takes no longer than a second.

Do you have any thoughts on this, or possible solutions?

One idea: Could we preserve the code output, and maybe have it contain some delimiters that mark the beginning and ending of each individual module. Then we could just escodegen the modules that change and replace their code within the precompiled bundle.

vendethiel commented 11 years ago

I use Sugar in its minified version, and the escodegen takes ~280ms. This is not "nothing", but is still correct. (See my project here)

michaelficarra commented 11 years ago

@Nami-Doc: Thanks for confirming that the build is not taking very long on your machine.

@krisnye: Thanks for the well-written feature request, but I don't think I want to start using string concatenation when building an AST is more appropriate. Moving away from string manipulation was one of the main reasons for creating this project. Hopefully Constellation/escodegen or mozilla/source-map speed will improve over time. Closing as wontfix.

krisnye commented 11 years ago

No amount of performance improvement in escodegen will alter the fact that the current algorithm is O(n).

If you wish to revisit this in the future, then consider just having a one to one mapping from input files to multiple output files. This way, you will still have the clean AST modifications but you will also have the O(1) performance which is essential for larger projects.

michaelficarra commented 11 years ago

@krisnye: Multiple output files wouldn't work. The modules need to have a shared private scope to hide the require/process/global vars. They can't do this across files in the browser.

krisnye commented 11 years ago

Just set require/process/global on the window object. When you run node from the command line, you have all those global variables and you can test code and inspect modules freely, as if you were writing a module on the fly.

I was honestly expecting similar behavior from commonjs-everywhere initially.

The ability to load up a page with the modules and use the console debugger to interactively require modules and inspect global properties.

This way, you can also use your built browser library from any page, using whatever modules you need.

michaelficarra commented 11 years ago

@krisnye: Then in the module you bundle, module.exports = require. And use -x require. Now you've exposed the internal require and can do what you want with it. You can also do global.global = global; global.require = require, etc. to expose the others yourself. You just have to be explicit about it. I really like being explicit.

krisnye commented 11 years ago

@michaelficarra True enough, explicit is good. Realtime recompilation is better.

Here's the tradeoffs:

Use single file export:

Use multiple file export:

I like the tradeoff in the second option. For me, having a fast edit/compile/test loop is essential. My time is my life, and since I work from home, the more efficient I am, the more time I can spend doing things I want to do. The drawback of adding to the global namespace isn't even a drawback to me. I like having those there, and it seem unlikely that anyone using commonjs-everywhere is going to be thrown by their presence.

Your project isn't really commonjs-everywhere anyways. You are porting nodejs specific modules. Your project is more correctly called nodejs-everywhere. And nodejs provides those global variables, so it's reasonable and consistent to provide them in the browser as well.

michaelficarra commented 11 years ago

@krisnye: That's why there's the --no-node option. --node is enabled by default since most people would want it.

krisnye commented 11 years ago

@michaelficarra That's true. Still didn't address the O(n) performance though. If your "incremental build" time is O(n), then it's not really even worth calling it incremental is it?

If you don't address this then you should make clear to potential users that their incremental build time will grow in time as their projects grow.

I hope you do decide to address it though because I want this project to thrive. Realtime building is awesome. You cannot go back to "just a few seconds" after experiencing it.

vendethiel commented 11 years ago

Well, my machine has a perf indice (windows' one) of 6 and it runs smoothly, but on this computer, I admit it's slow :(. (I get a 3.5). CJS rebuilding takes 229ms but re-generation JS from the AST takes 1.5s :/.

krisnye commented 11 years ago

Yes, the rebuilding part is incremental, but the JS re-generation step is proportional to the size of your project. The solution is to individually build modules.

michaelficarra commented 11 years ago

@krisnye: How do you suppose we'd preserve source maps then? This project has no business jamming together strings of code or calculating source map offsets.

krisnye commented 11 years ago

@michaelficarra leave each module in it's own file. Then your development main entry point can just include the other scripts like so:

document.writeln("<script src='/js/modules/require.js'></script>");
document.writeln("<script src='/js/modules/browser-build/index.js'></script>");
document.writeln("<script src='/js/modules/browser-build/require.js'></script>");
document.writeln("<script src='/js/modules/browser-build/utility.js'></script>");
document.writeln("<script src='/js/modules/browser-build/watcher.js'></script>");
document.writeln("<script src='/js/modules/sugar/index.js'></script>");

You can leave the source/map merging step to some other tool that is focused on building release versions.

vendethiel commented 11 years ago

No, that's not an even option for me. I'm building an userscript.

krisnye commented 11 years ago

Then have two modes: development with quick incremental recompile, and release.

krisnye commented 11 years ago

You clearly understand the problem. You wrote all of your own code on this project explicitly to be able to do incremental updates. That's why you cache your intermediate representation no? If 100% of the system is not incremental, then the system is not incremental though. I wouldn't fix it either if I was you, provided my projects were all fairly small.

For large commercial projects though, this just becomes progressively worse.

tarruda commented 10 years ago

@krisnye you should try my fork, it does simple string and source map concatenation without escodegen

sokra commented 10 years ago

You can add a incremental mode to escodegen...

Additional parameters would be an old ast and old result. The new ast can use parts of the old ast which result is only copied from the old result.

This would fit the design goals of the project and may be a benefit for many projects.

tarruda commented 10 years ago

@sokra that would surely be a better approach, maybe you'd like to open this issue on escodegen's tracker?

michaelficarra commented 10 years ago

@sokra: Yes, please open an escodegen issue for this.