fs: include more fs.stat*() optimizations by mscdex · Pull Request #11665 · nodejs/node

mscdex · 2017-03-03T03:47:39Z

stat*() optimizations including:

Move async *stat() functions to FillStatsArray() now used by the sync *stat() functions
Avoid creating fs.Stats instances for implicit async/sync *stat() calls used in various fs functions
Store reference to Float64Array data on C++ side for easier/faster access, instead of passing from JS to C++ on every async/sync *stat() call

Additionally, there are some further realpath*() optimizations, including:

Skip URL instance check for common (string) cases
Avoid regexp on non-Windows platforms when parsing the root of a path
Skip call to getOptions() in common case where no options is passed
Avoid hasOwnProperty()

Some benchmark results with all of the changes in this PR:

                                                                        improvement confidence      p.value
 fs/readfile.js concurrent=1 len=1024 dur=5                                  5.50 %            0.177524964
 fs/readfile.js concurrent=1 len=16777216 dur=5                             -0.26 %            0.860522023
 fs/readfile.js concurrent=10 len=1024 dur=5                                 7.57 %         ** 0.004268331
 fs/readfile.js concurrent=10 len=16777216 dur=5                            -0.35 %            0.374914593
 fs/bench-stat.js kind="fstat" n=200000                                      3.71 %         ** 0.002509243
 fs/bench-stat.js kind="lstat" n=200000                                      3.16 %         ** 0.001381068
 fs/bench-stat.js kind="stat" n=200000                                       4.13 %          * 0.019328676
 fs/readFileSync.js n=600000                                                20.27 %        *** 2.313943e-38
 fs/bench-statSync.js kind="fstatSync" n=1000000                             2.01 %            1.066663e-01
 fs/bench-statSync.js kind="lstatSync" n=1000000                             4.42 %        *** 4.163876e-06
 fs/bench-statSync.js kind="statSync" n=1000000                              3.46 %        *** 1.333101e-06
 fs/bench-realpathSync.js type="resolved" n=300000                          86.06 %        *** 3.681945e-42
 fs/bench-realpathSync.js type="relative" n=400000                          61.71 %        *** 1.349012e-54
 fs/bench-realpath.js type="resolved" n=30000                                6.00 %        *** 3.601045e-11
 fs/bench-realpath.js type="relative" n=100000                               7.61 %        *** 3.163175e-14
 module/module-loader.js useCache="false" fullPath="false" thousands=50      5.05 %        *** 1.426385e-19
 module/module-loader.js useCache="false" fullPath="true" thousands=50       5.15 %        *** 6.171543e-16

I'm initially marking this as semver-major because of the changes in fs.readFile(), fs.readFileSync(), etc. in how they retrieve stats for files/directories. If someone monkey patches fs.*sync*(), those monkey matched methods would no longer be called from those changed functions (fs.readFile(), etc.). If we feel we shouldn't be worried about this, then I will gladly remove the label.

CI: https://ci.nodejs.org/job/node-test-pull-request/6674/

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

fs

not-an-aardvark · 2017-03-03T07:10:55Z

For what it's worth, this will break some modules like mock-fs which rely on mutating process.binding('fs') and shimming everything to avoid touching the actual filesystem.

I acknowledge that process.binding('fs') is not a public API, so changes like this might be worthwhile anyway. I just want to point out that changes to the API of process.binding('fs') methods do currently have ecosystem impact.

mscdex · 2017-03-03T07:15:53Z

@not-an-aardvark Well, in a recent PR I was told not to worry about process.binding() changes *shrug*. See this comment and this comment.

not-an-aardvark · 2017-03-03T07:23:30Z

Fair enough. The reason I bring this up is that #11522 ended up causing tschaub/mock-fs#197 and breaking some builds. (I'm not trying to argue that this particular decision was justified/unjustified -- I just want to make sure that these decisions are made with full information on what they might break downstream.)

bnoordhuis

LGTM with comments.

bnoordhuis · 2017-03-03T10:06:30Z

lib/fs.js

Missing dot. Happens in quite a few other places. I won't point them out but please fix them up.

bnoordhuis · 2017-03-03T10:10:06Z

lib/fs.js

Why the extra parens?

bnoordhuis · 2017-03-03T10:15:26Z

lib/fs.js

You could move this to after the if statement if you make it compare on the statValues entry directly.

bnoordhuis · 2017-03-03T10:18:41Z

lib/fs.js

Some field index constants would be good to have. Using number literals obscures it too much.

bnoordhuis · 2017-03-03T10:26:36Z

src/node_stat_watcher.cc

You can simplify this now to:

Local<Value> arg = Integer::New(env->isolate(), status); wrap->MakeCallback(env->onchange_string(), 1, &arg);

bnoordhuis · 2017-03-03T10:27:57Z

lib/fs.js

You can drop the constants. now here.

bnoordhuis · 2017-03-03T10:28:09Z

lib/fs.js

bnoordhuis · 2017-03-03T10:28:20Z

lib/fs.js

bnoordhuis · 2017-03-03T10:30:44Z

src/node_file.cc

jasnell · 2017-03-03T15:05:56Z

lib/fs.js

btw, it's worth noting that I had tried to convert this to a ES6 Class just to see what would happen and it yielded a 50% perf reduction.

jasnell

LGTM once @bnoordhuis' comments are addressed

mscdex · 2017-03-03T20:56:49Z

Updated.

CI: https://ci.nodejs.org/job/node-test-pull-request/6694/

addaleax

LGTM with a suggestion

addaleax · 2017-03-04T16:27:08Z

lib/fs.js

It might be good to leave a comment to indicate that these values refer to .nlink

In another comment I suggested adding constants for the field indices, that would help with legibility.

Changed here as well now. I misunderstood the original suggestion for this line.

bnoordhuis

LGTM with a suggestion.

bnoordhuis · 2017-03-04T16:50:59Z

lib/fs.js

In another comment I suggested adding constants for the field indices, that would help with legibility.

mscdex · 2017-03-06T02:07:39Z

CI again after making suggested changes: https://ci.nodejs.org/job/node-test-pull-request/6719/

bnoordhuis · 2017-03-06T09:44:16Z

What I had in mind was more along the lines of const kInoFieldIndex = ... declarations at the top of the file and use those consistently everywhere, with a const kStatFieldCount = 14 so you can use statValues[kStatFieldCount + kInoFieldIndex] to index into the second stat values list.

mscdex · 2017-03-06T09:53:59Z

@bnoordhuis I was trying to avoid lookups where possible as I'm not sure V8 will always inline the value, even if const.

bnoordhuis · 2017-03-06T09:59:53Z

Valid concern. One more argument for reintroducing macros, I suppose. Alternatively, you could write them as statValues[/* ino */ 7].

Fishrock123 · 2017-03-06T15:37:30Z

For what it's worth, this will break some modules like mock-fs which rely on mutating process.binding('fs') and shimming everything to avoid touching the actual filesystem.

I acknowledge that process.binding('fs') is not a public API, so changes like this might be worthwhile anyway. I just want to point out that changes to the API of process.binding('fs') methods do currently have ecosystem impact.

We should warn them ahead of time still I think. @mscdex do you figure the new way is still shimmable?

mscdex · 2017-03-06T17:37:50Z

@Fishrock123 Support for the Object::New()/BuildStatsObject() code paths for *statSync() was already removed in #11522 because it was decided we aren't supporting process.binding() changes.

mscdex · 2017-03-11T05:12:09Z

Rebased and added inline comments next to the Float64Array's indices.

Any further comments/questions/suggestions @nodejs/collaborators?

jasnell · 2017-03-15T00:57:58Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/6849/

Including: * Move async *stat() functions to FillStatsArray() now used by the sync *stat() functions * Avoid creating fs.Stats instances for implicit async/sync *stat() calls used in various fs functions * Store reference to Float64Array data on C++ side for easier/faster access, instead of passing from JS to C++ on every async/sync *stat() call PR-URL: #11665 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Including: * Skip URL instance check for common (string) cases * Avoid regexp on non-Windows platforms when parsing the root of a path * Skip call to `getOptions()` in common case where no `options` is passed * Avoid `hasOwnProperty()` PR-URL: #11665 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell · 2017-03-15T04:58:54Z

Landed in 6a5ab5d...7109774

Including: * Move async *stat() functions to FillStatsArray() now used by the sync *stat() functions * Avoid creating fs.Stats instances for implicit async/sync *stat() calls used in various fs functions * Store reference to Float64Array data on C++ side for easier/faster access, instead of passing from JS to C++ on every async/sync *stat() call PR-URL: nodejs#11665 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Including: * Skip URL instance check for common (string) cases * Avoid regexp on non-Windows platforms when parsing the root of a path * Skip call to `getOptions()` in common case where no `options` is passed * Avoid `hasOwnProperty()` PR-URL: nodejs#11665 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

kunalspathak · 2017-03-21T16:17:07Z

src/node_file.cc

+    fields = new double[2 * 14];
+    env->set_fs_stats_field_array(fields);
+  }
+  Local<ArrayBuffer> ab = ArrayBuffer::New(env->isolate(),


@mscdex , Correct me if i am wrong, but ArrayBuffer::New() creates a buffer by default in externalizeMode. Who frees fields array in that case? May be you want to pass ArrayBufferCreationMode::kExternalized to this API?

That is the default mode already? fields should never be freed by V8 according to the description, which is what we want.

Right, but then how/when do we delete fields?

May be you want to pass ArrayBufferCreationMode::kExternalized to this API?

What i meant was to pass ArrayBufferCreationMode::kInternalized instead.

We don't. It's created once and stays for the life of the process, for whenever someone calls the fs.stat*() functions implicitly or explicitly.

mscdex · 2017-04-15T20:28:26Z

@nodejs/ctc How do you feel about the semver-major-ness of these changes? Do you think it is warranted? The reason I ask is that apparently in some situations certain fs.Stats fields are negative in JS land because the previous stat() functions were converting unsigned integers to signed integers (see #12419).

So either we could decide this PR really isn't semver-major and backport it as-is to v7.x which still has async stat() calls returning fs.Stats created using the old method, or I extract the async stat() changes from this PR separately and only backport that to v7.x. The downside to the latter solution is that it might add confusion due to the partial backport-ness.

Thoughts?

addaleax · 2017-04-15T20:35:38Z

You mean, using this as a bugfix for #12419? I would be okay with that, but it’s probably also worth mentioning that #11522 made it into v6.10.1, so for being consistent we should also apply it there (or, alternatively, revert #11522 on v6.x).

mscdex · 2017-04-15T20:36:07Z

@addaleax Ah ok I did not know it made it into v6.x.

jasnell · 2017-04-15T20:39:20Z

If there have been no regressions I'm good with downgrading thing to semver-minor and backporting.

mscdex · 2017-04-15T20:44:46Z

@jasnell Have enough people tested with master though since this was landed?

jasnell · 2017-04-15T20:48:52Z

Unsure. Preferably it would sit through at least one release on current before backporting.

mscdex · 2017-04-15T20:55:59Z

I think we'd need to make a decision sooner than that because of the issue I linked to.

jasnell · 2017-04-15T20:58:03Z

Doh, actually nevermind lol. For some reason I was reading that as backporting to v6. I'm good with backporting to v7.

mscdex · 2017-04-15T21:02:50Z

Well it's the same situation for v6 really, unless we want to decide differently for that branch.

jasnell · 2017-04-15T21:10:03Z

Yep, understood. Let's get it into a v7 release. Give it a week or two, then decide for v6.

Including: * Move async *stat() functions to FillStatsArray() now used by the sync *stat() functions * Avoid creating fs.Stats instances for implicit async/sync *stat() calls used in various fs functions * Store reference to Float64Array data on C++ side for easier/faster access, instead of passing from JS to C++ on every async/sync *stat() call Backport-PR-URL: nodejs#11665 Fixes: nodejs#16496

Including: * Avoid regexp on non-Windows platforms when parsing the root of a path Backport-PR-URL: nodejs#11665

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. labels Mar 3, 2017

addaleax self-requested a review March 3, 2017 09:27

bnoordhuis approved these changes Mar 3, 2017

View reviewed changes

jasnell reviewed Mar 3, 2017

View reviewed changes

jasnell approved these changes Mar 3, 2017

View reviewed changes

mscdex force-pushed the fs-stat-perf-round2 branch 2 times, most recently from 69c14fc to 1fa23da Compare March 3, 2017 20:56

mscdex added this to the 8.0.0 milestone Mar 3, 2017

addaleax approved these changes Mar 4, 2017

View reviewed changes

bnoordhuis approved these changes Mar 4, 2017

View reviewed changes

lib/fs.js Outdated

Copy link

Member

bnoordhuis Mar 4, 2017

Choose a reason for hiding this comment

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

In another comment I suggested adding constants for the field indices, that would help with legibility.

jasnell approved these changes Mar 4, 2017

View reviewed changes

mscdex force-pushed the fs-stat-perf-round2 branch 2 times, most recently from e072aa3 to add40be Compare March 6, 2017 01:44

mscdex force-pushed the fs-stat-perf-round2 branch from add40be to 721a107 Compare March 11, 2017 05:11

jasnell closed this Mar 15, 2017

kunalspathak reviewed Mar 21, 2017

View reviewed changes

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

mscdex deleted the fs-stat-perf-round2 branch April 5, 2017 04:20

addaleax added the dont-land-on-v4.x label Apr 15, 2017

mscdex mentioned this pull request Apr 15, 2017

fs.stat and fs.statSync may not return identical struct #12419

Closed

mscdex mentioned this pull request Apr 24, 2017

backport: 11665 to v7 #12614

Closed

3 tasks

AGrzes mentioned this pull request Jun 5, 2017

mock-fs 4.3.0 broken on node 8 tschaub/mock-fs#208

Closed

JLHwung added a commit to JLHwung/node that referenced this pull request Nov 1, 2017

fs: more realpath*() optimizations

89048c2

Including: * Avoid regexp on non-Windows platforms when parsing the root of a path Backport-PR-URL: nodejs#11665

JLHwung mentioned this pull request Nov 1, 2017

backport: 11665 to v6 #16667

Closed

4 tasks

joyeecheung mentioned this pull request Apr 12, 2018

fs.Stats mtime/atime/ctime rounding differences between node.js 7.x and 8.x #19897

Closed

Uh oh!

Conversation

mscdex commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

not-an-aardvark commented Mar 3, 2017

Uh oh!

mscdex commented Mar 3, 2017

Uh oh!

not-an-aardvark commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

mscdex commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Mar 6, 2017

Uh oh!

bnoordhuis commented Mar 6, 2017

Uh oh!

mscdex commented Mar 6, 2017

Uh oh!

bnoordhuis commented Mar 6, 2017

Uh oh!

Fishrock123 commented Mar 6, 2017

Uh oh!

mscdex commented Mar 6, 2017

Uh oh!

mscdex commented Mar 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Mar 15, 2017

Uh oh!

jasnell commented Mar 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Mar 3, 2017 •

edited

Loading

not-an-aardvark commented Mar 3, 2017 •

edited

Loading

mscdex commented Mar 3, 2017 •

edited

Loading

mscdex Mar 6, 2017 •

edited

Loading

mscdex commented Mar 11, 2017 •

edited

Loading

mscdex Mar 21, 2017 •

edited

Loading