perf: cache file stats/reads #126
Conversation
There was a problem hiding this comment.
Hi, sorry for not getting back to you earlier. I think this looks really nice. I've been checking for issues this might be causing and the only thing I could come up is if an npm install happens while in watch mode which changes the availability or position of files in node_modules. But I guess the prudent user will know to restart watch mode in such a situation anyway so I guess this could be ignored for now. Great work!
|
Yes the other issue is if a file gets into an error state, then that error state is cached indefinitely. I think I'll address some of these concerns though as Im not entirely comfortable merging this PR in this state. |
keithamus
force-pushed
the
cache-reads
branch
2 times, most recently
from
6ccdb4f
to
0c3689c
Compare
|
@lukastaegert would be good to get your thoughts about this new change. I've changed two things since last review:
|
keithamus
force-pushed
the
cache-reads
branch
from
0c3689c
to
80f10a4
Compare
|
Thanks for your efforts here! Would be nice if you could rebase against current master as this will add a "prepare" script that should enable direct installation from Github. This should make it easier to test this PR in different scenarios. |
keithamus
force-pushed
the
cache-reads
branch
from
80f10a4
to
911c82c
Compare
|
Rebased 😸 |
src/index.js
Outdated
| err => { | ||
| if (err.code == 'ENOENT') return false; | ||
| throw err; | ||
| } |
There was a problem hiding this comment.
Why not just put this into the catch block below? Also, using both the second then argument and catch seems a little over the top to me... 😉
src/index.js
Outdated
| isFileCache = {}; | ||
| readFileCache = {}; | ||
| }, | ||
|
|
There was a problem hiding this comment.
Very good idea but you probably mean onwrite
keithamus
force-pushed
the
cache-reads
branch
from
911c82c
to
27d798f
Compare
|
Fixed up those issues. I'm comfortable enough with this now. We've been testing this internally for a while now and it works well. |
|
Hey @keithamus, sorry for not getting back to you. So it seems that while I do not have the npm publishing rights for this plugin, you do have them. So I was wondering if you would want to take it upon you to release the latest PRs for this plugin? |
|
For sure - are you happy with each of these PRs? |
|
Yup, thanks for taking care of this 👍! This really helps @guybedford and myself concentrate on maintaining rollup core. If you want a review from us for anything, don't hesitate to add us as reviewers. |
|
By the way, at the moment I am fledging out a solution to get precise performance timings for rollup both for internal code as well as time spend in plugin hooks rollup/rollup#2045. When bundling rollup with rollup-plugin-node-resolve@3.0.0, rollup spends about 180ms resolving ids on my machine. With rollup-plugin-node-resolve@3.2.0, this drops to 4ms! Amazing work indeed! 🎉 |
This is some work towards fixing (not entirely) #121. File reads and stat calls are cached for the lifetime of the code execution. This takes the number of times files are stated dramatically down (on our build, one file goes from 13,000 stat calls to 1,300), however
browser-resolvestill does many file reads/stats using its own mechanism - so further PRs will be arriving next week around this, as files are still read multiple times when they don't need to be.It may be worth clearing the read file cache out on an
ongeneraterollup call or something?Thoughts/comments welcome.
/cc @mislav who worked on this with me