Skip to content
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

Config caching #5292

Closed
wants to merge 2 commits into from
Closed

Config caching #5292

wants to merge 2 commits into from

Conversation

akx
Copy link

@akx akx commented Feb 10, 2017

Q A
Patch: Bug Fix? yes
Major: Breaking Change? no
Minor: New Feature? yes (kinda)
Deprecations? no
Spec Compliancy?
Tests Added/Pass? no/pass
Fixed Tickets Improves #3511
License MIT
Doc PR no
Dependency Changes no

I've been debugging some build slowness issues with Node's --trace-sync-io flag and I noticed build-config-chain.js ends up re-re-re-reading the same configuration file(s) over and over again, and as it's using sync IO, the Node.js event loop gets blocked.

This PR adds filename-keyed in-memory caching for configuration (package.json/.babelrc) and ignore (.babelignore) files.

This should be backwards-compatible, aside from processes which modify or create Babel configuration or ignore files during the lifetime of the builder process. I think that's a corner case not worth worrying too much about.

Performance impact

For the build I'm debugging, this seems to remove some 1,500 sync call warnings:

$ grep "sync API" babel*txt | cut -f 1 -d: | sort | uniq -c
10625 babel-post-patch.txt
11980 babel-pre-patch.txt

Most of the remaining sync call warnings are from babel-loader, for which there's a similar PR (babel/babel-loader#375).

To be somewhat more exact using summarize-sync-io, Babel used to do

$ node summarize-sync-io.js < babel-pre-patch.txt  | grep babel-core
###########....       1542 fs.fstatSync by babel-core/lib/transformation/file/options/build-config-chain.js:addConfig
###########....       1542 fs.readSync by babel-core/lib/transformation/file/options/build-config-chain.js:addConfig
###########....       1542 fs.openSync by babel-core/lib/transformation/file/options/build-config-chain.js:addConfig
###............        263 fs.existsSync by babel-core/lib/transformation/file/options/build-config-chain.js:exists
#..............         14 fs.lstatSync by babel-core/lib/helpers/resolve.js:exports.default

and with this patch, it does

$ node summarize-sync-io.js < babel-post-patch.txt  | grep babel-core
###############        263 fs.existsSync by babel-core/lib/transformation/file/options/build-config-chain.js:exists
###............         32 fs.readSync by babel-core/lib/transformation/file/options/build-config-chain.js:addConfig
###............         32 fs.openSync by babel-core/lib/transformation/file/options/build-config-chain.js:addConfig
###............         32 fs.fstatSync by babel-core/lib/transformation/file/options/build-config-chain.js:addConfig
##.............         14 fs.lstatSync by babel-core/lib/helpers/resolve.js:exports.default

for this particular build. :)

As for memory usage impact, I expect it to be negligible -- adding eslintignore files to the cache has to be peanuts all considered.

This avoids repeated `fs.readFileSync` calls.
Like configs, ignore files are only read once now.
@mention-bot
Copy link

@akx, thanks for your PR! By analyzing the history of the files in this pull request, we identified @hzoo, @jamestalmage and @SimenB to be potential reviewers.

@xtuc
Copy link
Member

xtuc commented Feb 10, 2017

Good catch!

I actually also noticed that while tracing the Node process. Babel looks at all directories to find a .babelrc file, in most cases this will end with ENOENT (No such file or directory).

I think we could cache this information as well.

I don't expect much a perfomance boost since no JSON parsing is involved.

Should we and how cache the result of a .babeljs.rc (#4892) file. It may be dynamic.

@akx
Copy link
Author

akx commented Feb 10, 2017

@xtuc Cheers :)

Mm, you mean caching which .babelrc should be used for each input file? If so, that's not actually necessary – a quick visual grep with a tactical console.log() seemed to confirm that a config chain is only created once per input file.
However, we could cache configurations per-directory – there's no way different files in the same directory could have different configurations, right?

Copy link
Member

@danez danez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@danez danez added area: perf PR: Polish 💅 A type of pull request used for our changelog categories labels Feb 14, 2017
@xtuc
Copy link
Member

xtuc commented Feb 15, 2017

@akx no sorry, I meant that we could store directories where there is no .babelrc.

@loganfsmyth
Copy link
Member

I missed this PR entirely :P I'd be curious to see what #5608 does to these stats, since it stats the files to check their mtime, but won't read the file content if the file has not changed, so it should be a lot faster.

@akx
Copy link
Author

akx commented Sep 12, 2017

So yeah, I guess this was superseded by #5608. Closing...

@akx akx closed this Sep 12, 2017
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: perf outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Polish 💅 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants