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

Cache file system operations #116

Merged
merged 3 commits into from Apr 17, 2017

Conversation

Projects
None yet
3 participants
@akx
Copy link
Contributor

akx commented Feb 10, 2017

While debugging some slowness in a build pipeline with Node's --trace-sync-io flag, I noticed browserslist significantly contributes to synchronous IO calls.

This patch set attempts to fix that, by:

  • 👉 Caching the isFile operations (2 sync IO calls per attempted file)
  • 👉 Caching all configuration parsing (sync IO + JSON/file parsing)

For situations where configuration may change within the process's lifetime, there's the BROWSERSLIST_DISABLE_CACHE environment variable, as well as a new browserslist.clearCaches() API.

Implicit current directory behavior

The low-level findConfig etc. functions implicitly used the current directory as the path. This is fixed in 44dacbc, but the higher-level browserslist() function still performs as before.

Performance impact

All said and done, this patch set has a significant effect on the number of sync IO calls for my CSS build and some wallclock effect too – see below. Since sync IO ties up the Node event loop, the improvements probably compound when attempting parallel builds.

In the chart and results below, test/perf-benchmark.js is a synthetic benchmark and gulp is the gulp css build of the project I'm working on.

screen shot 2017-02-16 at 10 50 14

Browserslist 1.7.3

$ time node test/perf-benchmark.js
        1.75 real         1.51 user         0.26 sys
$ time node ./node_modules/.bin/gulp css
[10:27:44] Finished 'css' after 4.09 s
        4.87 real         5.10 user         0.22 sys

File existence and content caching

This is the cache-exists-old branch, fwiw.

$ time node test/perf-benchmark.js
        0.27 real         0.28 user         0.01 sys
$ time node ./node_modules/.bin/gulp css
[10:28:20] Finished 'css' after 3.32 s
        4.11 real         4.43 user         0.15 sys

Full config caching

This is the current branch.

$ time node test/perf-benchmark.js
        0.10 real         0.09 user         0.01 sys
$ time node ./node_modules/.bin/gulp css
[10:29:11] Finished 'css' after 3.22 s
        4.00 real         4.33 user         0.14 sys
@ai

This comment has been minimized.

Copy link
Member

ai commented Feb 10, 2017

Wow. Looks great. I am right now in the middle of long travel. So I will look on this PR until Monday.

if I will forget, please ping me.

@akx

This comment has been minimized.

Copy link
Contributor

akx commented Feb 10, 2017

Cheers :) Have a great trip, @ai!

@ai

This comment has been minimized.

Copy link
Member

ai commented Feb 11, 2017

Yeap. eachParent current behaviour is a bug :(. We need to fix it.

@ai

This comment has been minimized.

Copy link
Member

ai commented Feb 11, 2017

My main concern — how to use clearCache()? Just “call a method” is not a solution ;). How we will use it in postcss-loader?

Maybe we should use mtime in parsePackage? But we need benchmark to be sure, that mtime is better.

@ai

This comment has been minimized.

Copy link
Member

ai commented Feb 11, 2017

So next steps:

  1. Could you fix eachParent on master? I will release it tomorrow.
  2. Benchmark to be sure, that performance become better. And to get real numbers.
  3. Try mtime cache invalidation on benchmark.
@akx

This comment has been minimized.

Copy link
Contributor

akx commented Feb 12, 2017

I created a new PR of the eachParent fix (with a better implementation to boot): #117.

To be honest, I don't think there'll be too many situations where you'd need to be able to clear the cache during a process's lifetime; I just added the API for those rare corner cases.

For what it's worth, Babel also has a similar configuration cache (and I have a similar PR there to improve it :) ) and it has no API to clear the cache during runtime.

Regarding mtime cache invalidation... If you mean we'd only re-read files when they've changed based on mtime, that'd mean we'd need to statSync every file, and we'd be back to square one, so to speak.

@ai

This comment has been minimized.

Copy link
Member

ai commented Feb 12, 2017

Hm. My main concern is webpack dev server. It keep everything in memory, so Autoprefixer will not take a new config until you restart it.

Babel cache invalidation ignoring is a good point. But let’s try to find some solution first :).

Yeap, mtime reading will take some time, but my point that reading it could be much faster than reading whole file. This is why any perfomance optimization should be started from creating a benchmark.

Also, what do you think if we will use time-based cache invalidation? Like “cache file content for 1 minute”. We could use it even in findConfig.

Main browserslist use case is gulp/webpack, where all tools (Autoprefixer, ESLint, Babel) run in same time. So main problem is that we read config again and again in same time. If you compile 100 files, you will have 100 cache reading. But using time-based cache invalidation we will have 1 cache read for 1 build step, which is much much better.

@ai

This comment has been minimized.

Copy link
Member

ai commented Feb 12, 2017

I created a poll for Autoprefixer users
https://twitter.com/autoprefixer/status/830796056216616960

@akx

This comment has been minimized.

Copy link
Contributor

akx commented Feb 12, 2017

The way I see it, it's not about how long an operation takes (though of course readFileSync will take longer than statSync), but the fact that it's a synchronous operation that ties up the Node event loop, so absolutely no other JS code (or other synchronous system or library calls) can run meanwhile.

Time-based cache invalidation should be fine, but on the other hand this is about configuration, not the code itself... You're likely not willing to wait for 1 minute for your configuration changes to take effect in a long-running (webpack-dev-server/gulp watch/...) process anyway -- it'd be easier to just document that browserslist configuration changes require restart of processes like that, I think?

@ben-eb

This comment has been minimized.

Copy link
Collaborator

ben-eb commented Feb 12, 2017

It might be good to be asynchronous by default and expose a browserslist.sync interface, instead.

@ai

This comment has been minimized.

Copy link
Member

ai commented Feb 12, 2017

I added a task for async API #118

We could move to it on next major release.

@ai

This comment has been minimized.

Copy link
Member

ai commented Feb 12, 2017

@akx yeap, Twitter poll agree with you. I will think about it for a day.

Also, what about benchmark?

@akx

This comment has been minimized.

Copy link
Contributor

akx commented Feb 12, 2017

@ai Any ideas for a methodology for a robust benchmark? I just have the anecdotal single build's number up there...

@ai

This comment has been minimized.

Copy link
Member

ai commented Feb 12, 2017

For example, let’s have this test projects:

one/
  app/
    css/
      test.css
  package.json
two/
  app/
    js/
      test.js
  browserslist
three/
  app/
    js/
      test.js

For benchmark you need to read 100 times one/app/css/test.css, 100 times two/app/js/test.js and 300 times three/app/js/test.js (because most of Autoprefixer use cases doesn’t have browsers config).

@akx akx force-pushed the akx:cache-exists branch from 873e455 to 0a2267a Feb 13, 2017

@akx

This comment has been minimized.

Copy link
Contributor

akx commented Feb 13, 2017

Okay, I added a small performance benchmark that runs 3000 findConfigs on each file (100/300 were too small for significant measuring):

~/b/browserslist (cache-exists) $ time env BROWSERSLIST_DISABLE_CACHE= node test/perf-benchmark.js
        0.35 real         0.30 user         0.05 sys
~/b/browserslist (cache-exists) $ time env BROWSERSLIST_DISABLE_CACHE=1 node test/perf-benchmark.js
        2.51 real         2.11 user         0.40 sys

@akx akx force-pushed the akx:cache-exists branch from 0a2267a to b9117a3 Feb 13, 2017

@akx

This comment has been minimized.

Copy link
Contributor

akx commented Feb 13, 2017

By the way, here's a real life benchmark regarding parallel performance, from the project I was debugging the slowness in in the first place:

Without patched Browserslist

$ node --trace-sync-io ./node_modules/.bin/gulp css copy-fa-fonts 2> css-copy.txt
[10:33:55] Starting 'css'...
[10:33:55] Starting 'copy-fa-fonts'...
[10:34:04] Finished 'css' after 8.94 s
[10:34:04] Finished 'copy-fa-fonts' after 8.88 s

With patched Browserslist

(basically deleted all transitive browserslists from node_modules, then yarn linked the patched version into place)

$ node --trace-sync-io ./node_modules/.bin/gulp css copy-fa-fonts 2> css-copy-2.txt
[10:40:45] Starting 'css'...
[10:40:45] Starting 'copy-fa-fonts'...
[10:40:50] Finished 'css' after 4.61 s
[10:40:50] Finished 'copy-fa-fonts' after 4.55 s

For the record, copy-fa-fonts finishes in 27 ms if invoked by itself, so the CSS task (a standard LESS/postcss/cssnano/sourcemaps pipeline) is holding it back. :)

@ai

This comment has been minimized.

Copy link
Member

ai commented Feb 13, 2017

@akx great result!

@ai

This comment has been minimized.

Copy link
Member

ai commented Feb 13, 2017

What do you think about caching a findConfig. I think looking into many directories takes more time than reading one file.

@ai

This comment has been minimized.

Copy link
Member

ai commented Feb 13, 2017

After a thinking and twitter poll a agree that we should not clear cache.

Next concern — we should release it as major update. So it could take a while (sorry for long process with this task, such big changes should have good discussion).

@akx

This comment has been minimized.

Copy link
Contributor

akx commented Feb 13, 2017

I can experiment with wholesale caching of findConfig (per-directory).

And no worries, I totally understand this is a semver major update :)

@ai

This comment has been minimized.

Copy link
Member

ai commented Feb 13, 2017

Yeap, my main concern about adding benchmark is that findConfig could be a biggest performance problem.

@akx

This comment has been minimized.

Copy link
Contributor

akx commented Feb 13, 2017

I experimented a little (see akx@00e5836), but it looks like there's no additional performance gain to be had from just caching findConfig() in the real-life gulp example, but test/perf-benchmark.js is significantly faster still:

(cache-exists) $ time env BROWSERSLIST_DISABLE_CACHE= node test/perf-benchmark.js
        0.37 real         0.33 user         0.05 sys
(cache-config) $ time env BROWSERSLIST_DISABLE_CACHE= node test/perf-benchmark.js
        0.10 real         0.08 user         0.01 sys

That patch is slightly less complex too (less places where things get cached, though on the other hand the false/cwd code from eachParent has to be duplicated in findConfig()) – I'd maybe go with it. What do you think?

@ai

This comment has been minimized.

Copy link
Member

ai commented Feb 13, 2017

Do you have browserslift config in you gulp example?

@akx

This comment has been minimized.

Copy link
Contributor

akx commented Feb 13, 2017

I actually just had autoprefixer({browsers: ['last 2 versions']}), in the gulpfile. (I think Autoprefixer shouldn't do the findConfig dance if the configuration has been explicitly passed in to the module, but that's another bug ;) )

I got rid of that and added the "browserslist": ["last 2 versions"] stanza in package.json – still, no measurable difference there between this branch and cache-config.

@ai

This comment has been minimized.

Copy link
Member

ai commented Feb 13, 2017

The main case of using Autoprefixer is to not have a browsers option or browserslist config. This is why benchmark is so important.

@akx akx force-pushed the akx:cache-exists branch from b9117a3 to 00e5836 Feb 14, 2017

@akx

This comment has been minimized.

Copy link
Contributor

akx commented Feb 14, 2017

@ai I replaced the old implementation in this branch with the cache-config one, since it's simpler and faster. Can you re-review?

The old code is still available in my repo as cache-exists-old.

@ai

This comment has been minimized.

Copy link
Member

ai commented Feb 14, 2017

Old implemention of what? Sorry, I am without laptop and very hungry right now. It is hard to think.

@akx

This comment has been minimized.

Copy link
Contributor

akx commented Feb 14, 2017

The implementation in this branch now caches findConfig's results, as you suggested, instead of the old implementation which cached the results of file read operations. That is, the caching happens at a higher level, so it's even faster.

@ai

This comment has been minimized.

Copy link
Member

ai commented Feb 14, 2017

What benchmark tells? Benchmark is first in performance optimizations.

@ai

This comment has been minimized.

Copy link
Member

ai commented Feb 14, 2017

But code looks very good.

My main concern that all performance optimization should be done with connection to benchmark. So after every change, we should compare benchmark results.

@akx

This comment has been minimized.

Copy link
Contributor

akx commented Feb 15, 2017

As mentioned some comments above, cache-config (which is this branch now) had no measurable difference in the real life Gulp test, but the standalone benchmark got understandably lots faster, because it only does any work once, no matter how many iterations.

Anyway --

Is the user supposed to pass in a directory or a filename to findConfig?

As it is, if you pass in a directory, any configuration in that directory won't be considered, as eachParent only iterates through parents of the passed-in path.

Also, if this will end up being a semver-major update, I think it would be worth it to drop the implicit current-directory behavior?

@ai

This comment has been minimized.

Copy link
Member

ai commented Feb 15, 2017

As mentioned some comments above

Can you show a real number?

Is the user supposed to pass in a directory or a filename to findConfig?

By API user should pass a file in path option (so in findConfig too). But some user could pass a dir too, so extra check is better :).

Also, if this will end up being a semver-major update, I think it would be worth it to drop the implicit current-directory behavior?

What do you mean about “current-directory behavior”.

@akx

This comment has been minimized.

Copy link
Contributor

akx commented Feb 15, 2017

By API user should pass a file in path option (so in findConfig too). But some user could pass a dir too, so extra check is better :).

Alright.

What do you mean about “current-directory behavior”.

If you pass undefined to findConfig(), it will try and look in the current directory (though, as it is now, it will look in the current directory's parents ...)

I think it'd be clearer to require the user to explicitly pass in a path rather than "guess" it.

@ai

This comment has been minimized.

Copy link
Member

ai commented Feb 15, 2017

Sure, let’s disable current-directory behavior. I never plan to have it, it was just code mistake :).

akx added some commits Feb 13, 2017

Add simple performance benchmark based on @ai's suggestion
Usage:

```
time env BROWSERSLIST_DISABLE_CACHE=1 node test/perf-benchmark.js
time env BROWSERSLIST_DISABLE_CACHE= node test/perf-benchmark.js
```
Cache file existence and configuration objects
Since file existence checks and configuration reads are done
synchronously, those calls tie up the Node.js event loop.
Always require an actual path to be passed to findConfig and friends
This does not change the behavior of the top-level `browserslist` function;
it will still implicitly use the working directory as `path`.

@akx akx force-pushed the akx:cache-exists branch from 00e5836 to 520d69a Feb 16, 2017

@ai ai merged commit 9945555 into browserslist:master Apr 17, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment