ClojureScript

Significant code reload slowdown with :npm-deps

Details

  • Type: Enhancement Enhancement
  • Status: Resolved Resolved
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: 1.9.908
  • Fix Version/s: Next
  • Component/s: None
  • Labels:
  • Patch:
    Code
  • Approval:
    Accepted

Description

After migrating our dependencies from cljsjs to node_modules we noticed that figwheel took a lot longer to reload our code.

I asked in #clojurescript if anyone had an idea of what could be done and @anmonteiro wrote:
@petterik might just be because we're processing all your node dependencies through Closure on every reload feel free to open a JIRA ticket, we could probably employ a caching strategy somewhere

Versions used:
clojurescript "1.9.908"
lein-figwheel "0.5.13"

npm-depm deps:
:react-helmet "5.1.3"
:react "15.6.1"
:react-dom "15.6.1"
as well as some devDependencies in package.json.

  1. CLJS-2339.patch
    17/Sep/17 11:35 PM
    1 kB
    António Nuno Monteiro
  2. CLJS-2339-2.patch
    20/Sep/17 5:57 PM
    8 kB
    António Nuno Monteiro

Activity

Hide
Petter Eriksson added a comment -

If needed to repro, here are the additional devDependencies:
"phantomjs": "1.9.19",
"foundation-cli": "2.2.3",
"bower": "1.8.0"

Show
Petter Eriksson added a comment - If needed to repro, here are the additional devDependencies: "phantomjs": "1.9.19", "foundation-cli": "2.2.3", "bower": "1.8.0"
Hide
David Nolen added a comment -

This ticket will need more information. It might just be a Figwheel issue.

Show
David Nolen added a comment - This ticket will need more information. It might just be a Figwheel issue.
Hide
Tony Kay added a comment - - edited

OK, so I have some additional interesting facts. It does compile and work (where "work" is defined as "renders a thing from the npm module").

cljs: 1.9.908
cljsbuild: 1.1.7 via lein
Heap size: 2g
npm-deps: react, react-dom, and @blueprintjs/core
cljs deps: Om Next
verbose: true

Project is an om-next app, so cljsjs.react is in the classpath

Building the application without the npm deps and no code that refers to them: 46 seconds (CPU time)
Adding the NPM deps (with install true, but no code that uses them) increases this just be the amount of the install: 59 seconds
using the npm deps increases compile time to: 3 minutes 50 seconds

Critically: with verbose on, I can see that om/next.cljs takes about 15s in the first two cases, and 3 minutes in the final one. In other words: the thing that is slow is compiling next.cljc. Nothing else gets slower.

I'm not sure if this is even going to work "right" when it does compile, since I'm not sure how the cljsjs.react and npm react can co-exist (this is where I assume Om Next probably just needs to be made to use npm instead of cljsjs?)

But, since I saw that Petter was pulling in similar deps, he might be hitting a similar problem with cljsjs.react and npm react both being "in scope" so to speak.

When I use it from figwheel, the time between reloads becomes unusable. I assume it is related, but I have no data to back that up.

EDIT: I had missed the new doc on :global-exports. I'm going to try that and add my results.

Show
Tony Kay added a comment - - edited OK, so I have some additional interesting facts. It does compile and work (where "work" is defined as "renders a thing from the npm module"). cljs: 1.9.908 cljsbuild: 1.1.7 via lein Heap size: 2g npm-deps: react, react-dom, and @blueprintjs/core cljs deps: Om Next verbose: true Project is an om-next app, so cljsjs.react is in the classpath Building the application without the npm deps and no code that refers to them: 46 seconds (CPU time) Adding the NPM deps (with install true, but no code that uses them) increases this just be the amount of the install: 59 seconds using the npm deps increases compile time to: 3 minutes 50 seconds Critically: with verbose on, I can see that om/next.cljs takes about 15s in the first two cases, and 3 minutes in the final one. In other words: the thing that is slow is compiling next.cljc. Nothing else gets slower. I'm not sure if this is even going to work "right" when it does compile, since I'm not sure how the cljsjs.react and npm react can co-exist (this is where I assume Om Next probably just needs to be made to use npm instead of cljsjs?) But, since I saw that Petter was pulling in similar deps, he might be hitting a similar problem with cljsjs.react and npm react both being "in scope" so to speak. When I use it from figwheel, the time between reloads becomes unusable. I assume it is related, but I have no data to back that up. EDIT: I had missed the new doc on :global-exports. I'm going to try that and add my results.
Hide
Tony Kay added a comment -

So, I fixed the build to be "correct" with `:global-exports so that I only have the NPM version of react and react-dom around (excluded cljsjs/react and react-dom from Om Next). The compile time for next.cljc is still about 3 minutes (compared to the "normal" 15s)

BUT, I then removed blueprint from my `:npm-deps` (and usage), but kept react, react-dom, and a use of react (the NPM one) The time to compile next.cljc dropped to about a minute! Still 4x slower than "normal", but 4x faster than with blueprint in the npm deps. It seems that a file using an npm dep see a significant slowdown that is somehow proportional to the amount of total npm deps code "in view".

Obviously, Om Next uses React, but not blueprint. Yet, it's compile time is significantly affected.

What Antonio said about processing them all through Closure certainly sounds relevant. Kind of a let down to go from recent speed-ups in compiler speed to this sudden jolt of a performance hit when we finally get a better interface to libraries

Show
Tony Kay added a comment - So, I fixed the build to be "correct" with `:global-exports so that I only have the NPM version of react and react-dom around (excluded cljsjs/react and react-dom from Om Next). The compile time for next.cljc is still about 3 minutes (compared to the "normal" 15s) BUT, I then removed blueprint from my `:npm-deps` (and usage), but kept react, react-dom, and a use of react (the NPM one) The time to compile next.cljc dropped to about a minute! Still 4x slower than "normal", but 4x faster than with blueprint in the npm deps. It seems that a file using an npm dep see a significant slowdown that is somehow proportional to the amount of total npm deps code "in view". Obviously, Om Next uses React, but not blueprint. Yet, it's compile time is significantly affected. What Antonio said about processing them all through Closure certainly sounds relevant. Kind of a let down to go from recent speed-ups in compiler speed to this sudden jolt of a performance hit when we finally get a better interface to libraries
Hide
António Nuno Monteiro added a comment -

Patch attached.

Show
António Nuno Monteiro added a comment - Patch attached.
Hide
Tony Kay added a comment -

That patch fixes the build issue on plain cljsbuild.

Figwheel now starts quickly (first complete compile), but a simple file change on a small project takes 12s to hot code reload, making it pretty unusable.

Show
Tony Kay added a comment - That patch fixes the build issue on plain cljsbuild. Figwheel now starts quickly (first complete compile), but a simple file change on a small project takes 12s to hot code reload, making it pretty unusable.
Hide
António Nuno Monteiro added a comment -

So it looks like this is a 2-part problem.

The first one, which my patch solved, is related to CLJS compilation (where we were performing unnecessary computations on every symbol analysis – which slowed down proportionally to the number of JS modules processed).

The second problem is that we process every JS module on every code reload: the solution for this one is implementing a strategy for figuring out if we need to process JS modules again (e.g. based on last modified timestamps of source files, just like `cljs.compiler/requires-compilation?`).

Show
António Nuno Monteiro added a comment - So it looks like this is a 2-part problem. The first one, which my patch solved, is related to CLJS compilation (where we were performing unnecessary computations on every symbol analysis – which slowed down proportionally to the number of JS modules processed). The second problem is that we process every JS module on every code reload: the solution for this one is implementing a strategy for figuring out if we need to process JS modules again (e.g. based on last modified timestamps of source files, just like `cljs.compiler/requires-compilation?`).
Hide
António Nuno Monteiro added a comment -

The attached CLJS-2339-2.patch contains the changes in the previous patch and also solves the issues around reloading, only running the foreign libraries that are modules through Closure if any of the source files in the dependency graph have changed.

I'd appreciate if people who are seeing the issue can try out the patch and report back.

Show
António Nuno Monteiro added a comment - The attached CLJS-2339-2.patch contains the changes in the previous patch and also solves the issues around reloading, only running the foreign libraries that are modules through Closure if any of the source files in the dependency graph have changed. I'd appreciate if people who are seeing the issue can try out the patch and report back.
Hide
Tony Kay added a comment -

So, I did some profiling with visualvm, and gave the results to Antonio. They were showing the vast majority of the time being consumed by `pipe`, under the direction of the node module discovery functions. His new version of the second patch definitely improves reload speed considerably. My hot code reload went from about 14 seconds (via patch 1) to 2 seconds with the new version of patch 2. This is on a project with Om Next, and some largish node libraries.

Show
Tony Kay added a comment - So, I did some profiling with visualvm, and gave the results to Antonio. They were showing the vast majority of the time being consumed by `pipe`, under the direction of the node module discovery functions. His new version of the second patch definitely improves reload speed considerably. My hot code reload went from about 14 seconds (via patch 1) to 2 seconds with the new version of patch 2. This is on a project with Om Next, and some largish node libraries.

People

Vote (0)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: