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

fs: improve performance for sync stat() functions #11522

Merged
merged 1 commit into from
Feb 26, 2017

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Feb 23, 2017

Results from benchmarks:

                                                 improvement confidence      p.value
 fs/bench-statSync.js kind="fstatSync" n=1000000     13.51 %        *** 4.033815e-18
 fs/bench-statSync.js kind="statSync" n=1000000       7.09 %        *** 6.502451e-15
 fs/bench-statSync.js kind="lstatSync" n=1000000      7.43 %        *** 9.979362e-16

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

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

@mscdex mscdex 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. performance Issues and PRs related to the performance of Node.js. labels Feb 23, 2017
@nodejs-github-bot 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 Feb 23, 2017
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM. Is there value in keeping the sync non-float64array code paths in the binding methods?

@mscdex
Copy link
Contributor Author

mscdex commented Feb 23, 2017

@bnoordhuis I left them there for backwards compatibility for now, even though third-party use of process.binding() is frowned upon/not supported. *shrug*

@bnoordhuis
Copy link
Member

I left them there for backwards compatibility for now, even though third-party use of process.binding() is frowned upon/not supported.

I figured that was the reason but I'd worry that people construe that as a nod of approval that it's okay to use the bindings layer directly.

@mscdex
Copy link
Contributor Author

mscdex commented Feb 23, 2017

@bnoordhuis I don't mind either way. If most @nodejs/collaborators agree to remove support for the old method, then I will remove it.

src/node_file.cc Outdated
@@ -649,7 +696,14 @@ static void FStat(const FunctionCallbackInfo<Value>& args) {

int fd = args[0]->Int32Value();

if (args[1]->IsObject()) {
if (args[1]->IsFloat64Array()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation looks off here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@jasnell
Copy link
Member

jasnell commented Feb 24, 2017

The recently added deprecation policy makes it fairly clear that process.binding() is not covered under our API guarantees. I know that it's a recent change but I believe it gives sufficient coverage on this.

@mscdex mscdex force-pushed the fs-statsync-perf branch 2 times, most recently from ef8da93 to 0f3da51 Compare February 24, 2017 02:26
@mscdex
Copy link
Contributor Author

mscdex commented Feb 24, 2017

I've removed the non-Float64Array sync() branches.

@mscdex
Copy link
Contributor Author

mscdex commented Feb 24, 2017

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

CI does not seem green.
LGTM when it does.

@bnoordhuis
Copy link
Member

bnoordhuis commented Feb 24, 2017

fs.exists() and fs.existsSync() use binding.stat() directly, they'll need to be updated.

@mscdex
Copy link
Contributor Author

mscdex commented Feb 24, 2017

PR-URL: nodejs#11522
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@mscdex mscdex merged commit 22abb39 into nodejs:master Feb 26, 2017
@mscdex mscdex deleted the fs-statsync-perf branch February 26, 2017 02:05
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 27, 2017
PR-URL: nodejs#11522
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@italoacasas italoacasas mentioned this pull request Feb 27, 2017
not-an-aardvark added a commit to not-an-aardvark/mock-fs that referenced this pull request Mar 2, 2017
Node 7.7 changed the behavior of the `binding.{stat,lstat,fstat}` functions (see nodejs/node#11522). This updates the binding functions used in mock-fs to match the new Node binding behavior, while still maintaining compatibility with old Node versions. The new behavior is detected when the second argument to `binding.{stat,lstat,fstat}` is a `Float64Array`, which would be an invalid argument for previous versions of the binding.
not-an-aardvark added a commit to not-an-aardvark/mock-fs that referenced this pull request Mar 2, 2017
…haub#197)

Node 7.7 changed the behavior of the `binding.{stat,lstat,fstat}` functions (see nodejs/node#11522). This updates the binding functions used in mock-fs to match the new Node binding behavior, while still maintaining compatibility with old Node versions. The new behavior is detected when the second argument to `binding.{stat,lstat,fstat}` is a `Float64Array`, which would be an invalid argument for previous versions of the binding.
@jasnell
Copy link
Member

jasnell commented Mar 7, 2017

@nodejs/lts @mscdex ... should we pull this into v6.x-staging?

@mscdex
Copy link
Contributor Author

mscdex commented Mar 7, 2017

@jasnell Fine by me, it should still provide a speedup even though I haven't explicitly benchmarked v6.x.

Why the dont-land-on-v4.x?

@jasnell
Copy link
Member

jasnell commented Mar 7, 2017

like the other one, the don't land is largely precautionary

jasnell pushed a commit that referenced this pull request Mar 9, 2017
PR-URL: #11522
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
PR-URL: #11522
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
MylesBorins added a commit that referenced this pull request Mar 21, 2017
Notable changes

* performance: The performance of several APIs has been improved.
  - `Buffer.compare()` is up to 35% faster on average. (Brian White)
    #10927
  - `buffer.toJSON()` is up to 2859% faster on average. (Brian White)
    #10895
  - `fs.*statSync()` functions are now up to 9.3% faster on average.
    (Brian White) #11522
  - `os.loadavg` is up to 151% faster. (Brian White)
    #11516
  - `process.memoryUsage()` is up to 34% faster. (Brian White)
    #11497
  - `querystring.unescape()` for `Buffer`s is 15% faster on average.
    (Brian White) #10837
  - `querystring.stringify()` is up to 7.8% faster on average.
    (Brian White) #10852
  - `querystring.parse()` is up to 21% faster on average. (Brian White)
    #10874

* IPC:
  - Batched writes have been enabled for process IPC on platforms that
    support Unix Domain Sockets. (Alexey Orlenko)
    #10677
  - Performance gains may be up to 40% for some workloads.

* child_process:
  - `spawnSync` now returns a null `status` when child is terminated by
    a signal. (cjihrig) #11288
  - This fixes the behavior to act like `spawn()` does.

* http:
  - Control characters are now always rejected when using
    `http.request()`. (Ben Noordhuis)
    #8923
  - Debug messages have been added for cases when headers contain
    invalid values. (Evan Lucas)
    #9195

* node:
  - Heap statistics now support values larger than 4GB. (Ben Noordhuis)
    #10186

* timers:
  - Timer callbacks now always maintain order when interacting with
    domain error handling. (John Barboza)
    #10522

PR-URL: #11759
MylesBorins added a commit that referenced this pull request Mar 21, 2017
Notable changes

* performance: The performance of several APIs has been improved.
  - `Buffer.compare()` is up to 35% faster on average. (Brian White)
    #10927
  - `buffer.toJSON()` is up to 2859% faster on average. (Brian White)
    #10895
  - `fs.*statSync()` functions are now up to 9.3% faster on average.
    (Brian White) #11522
  - `os.loadavg` is up to 151% faster. (Brian White)
    #11516
  - `process.memoryUsage()` is up to 34% faster. (Brian White)
    #11497
  - `querystring.unescape()` for `Buffer`s is 15% faster on average.
    (Brian White) #10837
  - `querystring.stringify()` is up to 7.8% faster on average.
    (Brian White) #10852
  - `querystring.parse()` is up to 21% faster on average. (Brian White)
    #10874

* IPC:
  - Batched writes have been enabled for process IPC on platforms that
    support Unix Domain Sockets. (Alexey Orlenko)
    #10677
  - Performance gains may be up to 40% for some workloads.

* child_process:
  - `spawnSync` now returns a null `status` when child is terminated by
    a signal. (cjihrig) #11288
  - This fixes the behavior to act like `spawn()` does.

* http:
  - Control characters are now always rejected when using
    `http.request()`. (Ben Noordhuis)
    #8923
  - Debug messages have been added for cases when headers contain
    invalid values. (Evan Lucas)
    #9195

* node:
  - Heap statistics now support values larger than 4GB. (Ben Noordhuis)
    #10186

* timers:
  - Timer callbacks now always maintain order when interacting with
    domain error handling. (John Barboza)
    #10522

PR-URL: #11759
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
    Notable changes

    * performance: The performance of several APIs has been improved.
      - `Buffer.compare()` is up to 35% faster on average. (Brian White)
        nodejs/node#10927
      - `buffer.toJSON()` is up to 2859% faster on average. (Brian White)
        nodejs/node#10895
      - `fs.*statSync()` functions are now up to 9.3% faster on average.
        (Brian White) nodejs/node#11522
      - `os.loadavg` is up to 151% faster. (Brian White)
        nodejs/node#11516
      - `process.memoryUsage()` is up to 34% faster. (Brian White)
        nodejs/node#11497
      - `querystring.unescape()` for `Buffer`s is 15% faster on average.
        (Brian White) nodejs/node#10837
      - `querystring.stringify()` is up to 7.8% faster on average.
        (Brian White) nodejs/node#10852
      - `querystring.parse()` is up to 21% faster on average. (Brian White)
        nodejs/node#10874

    * IPC:
      - Batched writes have been enabled for process IPC on platforms that
        support Unix Domain Sockets. (Alexey Orlenko)
        nodejs/node#10677
      - Performance gains may be up to 40% for some workloads.

    * child_process:
      - `spawnSync` now returns a null `status` when child is terminated by
        a signal. (cjihrig) nodejs/node#11288
      - This fixes the behavior to act like `spawn()` does.

    * http:
      - Control characters are now always rejected when using
        `http.request()`. (Ben Noordhuis)
        nodejs/node#8923
      - Debug messages have been added for cases when headers contain
        invalid values. (Evan Lucas)
        nodejs/node#9195

    * node:
      - Heap statistics now support values larger than 4GB. (Ben Noordhuis)
        nodejs/node#10186

    * timers:
      - Timer callbacks now always maintain order when interacting with
        domain error handling. (John Barboza)
        nodejs/node#10522

    PR-URL: nodejs/node#11759

Signed-off-by: Ilkka Myller <[email protected]>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
    Notable changes

    * performance: The performance of several APIs has been improved.
      - `Buffer.compare()` is up to 35% faster on average. (Brian White)
        nodejs/node#10927
      - `buffer.toJSON()` is up to 2859% faster on average. (Brian White)
        nodejs/node#10895
      - `fs.*statSync()` functions are now up to 9.3% faster on average.
        (Brian White) nodejs/node#11522
      - `os.loadavg` is up to 151% faster. (Brian White)
        nodejs/node#11516
      - `process.memoryUsage()` is up to 34% faster. (Brian White)
        nodejs/node#11497
      - `querystring.unescape()` for `Buffer`s is 15% faster on average.
        (Brian White) nodejs/node#10837
      - `querystring.stringify()` is up to 7.8% faster on average.
        (Brian White) nodejs/node#10852
      - `querystring.parse()` is up to 21% faster on average. (Brian White)
        nodejs/node#10874

    * IPC:
      - Batched writes have been enabled for process IPC on platforms that
        support Unix Domain Sockets. (Alexey Orlenko)
        nodejs/node#10677
      - Performance gains may be up to 40% for some workloads.

    * child_process:
      - `spawnSync` now returns a null `status` when child is terminated by
        a signal. (cjihrig) nodejs/node#11288
      - This fixes the behavior to act like `spawn()` does.

    * http:
      - Control characters are now always rejected when using
        `http.request()`. (Ben Noordhuis)
        nodejs/node#8923
      - Debug messages have been added for cases when headers contain
        invalid values. (Evan Lucas)
        nodejs/node#9195

    * node:
      - Heap statistics now support values larger than 4GB. (Ben Noordhuis)
        nodejs/node#10186

    * timers:
      - Timer callbacks now always maintain order when interacting with
        domain error handling. (John Barboza)
        nodejs/node#10522

    PR-URL: nodejs/node#11759

Signed-off-by: Ilkka Myller <[email protected]>
Slayer95 pushed a commit to Slayer95/mock-fs that referenced this pull request Apr 27, 2017
…haub#197)

Node 7.7 changed the behavior of the `binding.{stat,lstat,fstat}` functions (see nodejs/node#11522). This updates the binding functions used in mock-fs to match the new Node binding behavior, while still maintaining compatibility with old Node versions. The new behavior is detected when the second argument to `binding.{stat,lstat,fstat}` is a `Float64Array`, which would be an invalid argument for previous versions of the binding.
@JLHwung JLHwung mentioned this pull request Nov 3, 2017
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants