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

fs: include more fs.stat*() optimizations #11665

Closed
wants to merge 2 commits into
base: master
from

Conversation

@mscdex
Contributor

mscdex commented Mar 3, 2017

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

This comment has been minimized.

Show comment
Hide comment
@not-an-aardvark

not-an-aardvark Mar 3, 2017

Member

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.

Member

not-an-aardvark commented Mar 3, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Mar 3, 2017

Contributor

@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.

Contributor

mscdex commented Mar 3, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@not-an-aardvark

not-an-aardvark Mar 3, 2017

Member

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.)

Member

not-an-aardvark commented Mar 3, 2017

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.)

@addaleax addaleax self-requested a review Mar 3, 2017

@bnoordhuis

LGTM with comments.

Show outdated Hide outdated lib/fs.js
Show outdated Hide outdated lib/fs.js
Show outdated Hide outdated lib/fs.js
Show outdated Hide outdated lib/fs.js
Show outdated Hide outdated src/node_stat_watcher.cc
Show outdated Hide outdated lib/fs.js
Show outdated Hide outdated lib/fs.js
Show outdated Hide outdated lib/fs.js
Show outdated Hide outdated src/node_file.cc
Show outdated Hide outdated lib/fs.js
@jasnell

jasnell approved these changes Mar 3, 2017

LGTM once @bnoordhuis' comments are addressed

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex
Contributor

mscdex commented Mar 3, 2017

@mscdex mscdex added this to the 8.0.0 milestone Mar 3, 2017

@addaleax

LGTM with a suggestion

Show outdated Hide outdated lib/fs.js
@bnoordhuis

LGTM with a suggestion.

Show outdated Hide outdated lib/fs.js
@jasnell

jasnell approved these changes Mar 4, 2017

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Mar 6, 2017

Contributor

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

Contributor

mscdex commented Mar 6, 2017

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

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Mar 6, 2017

Member

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.

Member

bnoordhuis commented Mar 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Mar 6, 2017

Contributor

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

Contributor

mscdex commented Mar 6, 2017

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

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Mar 6, 2017

Member

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

Member

bnoordhuis commented Mar 6, 2017

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

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Mar 6, 2017

Member

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?

Member

Fishrock123 commented Mar 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Mar 6, 2017

Contributor

@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.

Contributor

mscdex commented Mar 6, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Mar 11, 2017

Contributor

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

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

Contributor

mscdex commented Mar 11, 2017

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

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

@mcollina

LGTM

@mscdex mscdex added in progress and removed in progress labels Mar 11, 2017

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Mar 12, 2017

Contributor

Alright, I've fixed the botched rebase I previously did.

I have also now included some more realpath*() optimizations and added relevant benchmark results for those changes to the original post.

CI: https://ci.nodejs.org/job/node-test-pull-request/6803/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/640/

EDIT: CI again to verify flakiness of some tests: https://ci.nodejs.org/job/node-test-commit/8402/
EDIT 2: Looks like some C++ assertions happening due to these changes in CITGM, will look at it tomorrow.
EDIT 3: My guess is there is some fs re-evaulation going on, which should now be worked around.

CI: https://ci.nodejs.org/job/node-test-pull-request/6816/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/642/

Contributor

mscdex commented Mar 12, 2017

Alright, I've fixed the botched rebase I previously did.

I have also now included some more realpath*() optimizations and added relevant benchmark results for those changes to the original post.

CI: https://ci.nodejs.org/job/node-test-pull-request/6803/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/640/

EDIT: CI again to verify flakiness of some tests: https://ci.nodejs.org/job/node-test-commit/8402/
EDIT 2: Looks like some C++ assertions happening due to these changes in CITGM, will look at it tomorrow.
EDIT 3: My guess is there is some fs re-evaulation going on, which should now be worked around.

CI: https://ci.nodejs.org/job/node-test-pull-request/6816/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/642/

@mscdex mscdex added in progress and removed in progress labels Mar 12, 2017

mscdex added some commits Mar 12, 2017

fs: include more fs.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
fs: more 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()`

@mscdex mscdex removed the in progress label Mar 13, 2017

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Mar 13, 2017

Contributor

Alright, I've fixed the final graceful-fs-related issue.

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/646/

No more related CITGM failures!

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

Contributor

mscdex commented Mar 13, 2017

Alright, I've fixed the final graceful-fs-related issue.

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/646/

No more related CITGM failures!

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

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Mar 13, 2017

Contributor

CI is green except for an unrelated CI infrastructure failure. I'd like some re-reviews of the recent changes if possible.

Contributor

mscdex commented Mar 13, 2017

CI is green except for an unrelated CI infrastructure failure. I'd like some re-reviews of the recent changes if possible.

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Mar 13, 2017

Member

Still LGTM, good work @mscdex

Member

mcollina commented Mar 13, 2017

Still LGTM, good work @mscdex

@tschaub

This comment has been minimized.

Show comment
Hide comment
@tschaub

tschaub Mar 13, 2017

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.)

Thanks for bringing this up @not-an-aardvark - and for providing a fix.

Off topic for this PR I know, but I thought it would be worth mentioning that binding('fs') has proven to be a more stable interface (or at least easier to implement in a backwards compatible way) than the fs module itself based on personal experience having implemented it from Node 0.8 through Node 7.

Clearly others shouldn't rely on binding('fs') being stable (particularly between major releases). I just wanted to point out that historically it has been more "stable" than the fs module.

tschaub commented Mar 13, 2017

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.)

Thanks for bringing this up @not-an-aardvark - and for providing a fix.

Off topic for this PR I know, but I thought it would be worth mentioning that binding('fs') has proven to be a more stable interface (or at least easier to implement in a backwards compatible way) than the fs module itself based on personal experience having implemented it from Node 0.8 through Node 7.

Clearly others shouldn't rely on binding('fs') being stable (particularly between major releases). I just wanted to point out that historically it has been more "stable" than the fs module.

@jasnell

This comment has been minimized.

Show comment
Hide comment
Member

jasnell commented Mar 15, 2017

jasnell added a commit that referenced this pull request Mar 15, 2017

fs: include more fs.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

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 added a commit that referenced this pull request Mar 15, 2017

fs: more 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()`

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

This comment has been minimized.

Show comment
Hide comment
@jasnell
Member

jasnell commented Mar 15, 2017

Landed in 6a5ab5d...7109774

@jasnell jasnell closed this Mar 15, 2017

jungx098 added a commit to jungx098/node that referenced this pull request Mar 21, 2017

fs: include more fs.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

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>

jungx098 added a commit to jungx098/node that referenced this pull request Mar 21, 2017

fs: more 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()`

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

This comment has been minimized.

@kunalspathak

kunalspathak Mar 21, 2017

Contributor

@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?

@kunalspathak

kunalspathak Mar 21, 2017

Contributor

@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?

This comment has been minimized.

@mscdex

mscdex Mar 21, 2017

Contributor

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

@mscdex

mscdex Mar 21, 2017

Contributor

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

This comment has been minimized.

@kunalspathak

kunalspathak Mar 21, 2017

Contributor

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

@kunalspathak

kunalspathak Mar 21, 2017

Contributor

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

This comment has been minimized.

@kunalspathak

kunalspathak Mar 21, 2017

Contributor

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

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

@kunalspathak

kunalspathak Mar 21, 2017

Contributor

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

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

This comment has been minimized.

@mscdex

mscdex Mar 21, 2017

Contributor

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

mscdex Mar 21, 2017

Contributor

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.

This comment has been minimized.

@kunalspathak

kunalspathak Mar 21, 2017

Contributor

Got it.

@kunalspathak

kunalspathak Mar 21, 2017

Contributor

Got it.

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

@mscdex mscdex deleted the mscdex:fs-stat-perf-round2 branch Apr 5, 2017

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Apr 15, 2017

Contributor

@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?

Contributor

mscdex commented Apr 15, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Apr 15, 2017

Member

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).

Member

addaleax commented Apr 15, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Apr 15, 2017

Contributor

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

Contributor

mscdex commented Apr 15, 2017

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

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 15, 2017

Member

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

Member

jasnell commented Apr 15, 2017

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

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Apr 15, 2017

Contributor

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

Contributor

mscdex commented Apr 15, 2017

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

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 15, 2017

Member

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

Member

jasnell commented Apr 15, 2017

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

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Apr 15, 2017

Contributor

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

Contributor

mscdex commented Apr 15, 2017

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

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 15, 2017

Member

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

Member

jasnell commented Apr 15, 2017

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

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Apr 15, 2017

Contributor

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

Contributor

mscdex commented Apr 15, 2017

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

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 15, 2017

Member

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

Member

jasnell commented Apr 15, 2017

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

@mscdex mscdex referenced this pull request Apr 24, 2017

Closed

backport: 11665 to v7 #12614

3 of 3 tasks complete

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

fs: include more fs.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

Backport-PR-URL: nodejs#11665

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

fs: more realpath*() optimizations
Including:

* Avoid regexp on non-Windows platforms when parsing the root of a path

Backport-PR-URL: nodejs#11665

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

fs: include more fs.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

Backport-PR-URL: nodejs#11665
Fixes: nodejs#16496

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

fs: more realpath*() optimizations
Including:

* Avoid regexp on non-Windows platforms when parsing the root of a path

Backport-PR-URL: nodejs#11665

@JLHwung JLHwung referenced this pull request Nov 1, 2017

Closed

backport: 11665 to v6 #16667

3 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment