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: add a fast-path for readFileSync utf-8 #48658

Merged
merged 5 commits into from
Jul 12, 2023

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Jul 5, 2023

This pull request improves the performance of fs.readFileSync for UTF-8 encoding.

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1346

                                                                  confidence improvement accuracy (*)   (**)   (***)
fs/readFileSync.js n=600 path='existing' encoding='undefined'                    -2.98 %       ±3.46% ±4.61%  ±6.00%
fs/readFileSync.js n=600 path='existing' encoding='utf8'                 ***     72.60 %       ±6.46% ±8.64% ±11.32%
fs/readFileSync.js n=600 path='non-existing' encoding='undefined'                -1.16 %       ±2.29% ±3.05%  ±3.97%
fs/readFileSync.js n=600 path='non-existing' encoding='utf8'                      2.02 %       ±3.05% ±4.06%  ±5.28%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 4 comparisons, you can thus
expect the following amount of false-positive results:
  0.20 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.04 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jul 5, 2023
lib/fs.js Outdated Show resolved Hide resolved
@anonrig anonrig force-pushed the node-file branch 3 times, most recently from 3ecaadc to 1227128 Compare July 5, 2023 16:12
@anonrig anonrig added the performance Issues and PRs related to the performance of Node.js. label Jul 5, 2023
@anonrig
Copy link
Member Author

anonrig commented Jul 5, 2023

@santigimeno @bnoordhuis One of the tests failing and throwing ESPIPE: invalid seek, undefined error. Can you point me towards the faulty lines?

Stack trace
AssertionError [ERR_ASSERTION]: ifError got unwanted exception: Command failed: cat /Users/yagiz/Developer/node/test/.tmp.0/readfilesync_pipe_large_test.txt | "/Users/yagiz/Developer/node/out/Release/node" "/Users/yagiz/Developer/node/test/parallel/test-fs-readfilesync-pipe-large.js" child
node:internal/fs/utils:351
    throw err;
    ^

Error: ESPIPE: invalid seek, undefined
    at readFileSyncUtf8 (node:internal/fs/read-file/utf8:19:3)
    at Object.readFileSync (node:fs:467:12)
    at Object.<anonymous> (/Users/yagiz/Developer/node/test/parallel/test-fs-readfilesync-pipe-large.js:14:27)
    at Module._compile (node:internal/modules/cjs/loader:1233:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1287:10)
    at Module.load (node:internal/modules/cjs/loader:1091:32)
    at Module._load (node:internal/modules/cjs/loader:938:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:83:12)
    at node:internal/main/run_main_module:23:47 {
  errno: -29,
  code: 'ESPIPE'
}

Node.js v21.0.0-pre

    at ChildProcess.exithandler (node:child_process:419:12)
    at ChildProcess.emit (node:events:512:28)
    at maybeClose (node:internal/child_process:1105:16)
    at Socket.<anonymous> (node:internal/child_process:457:11)
    at Socket.emit (node:events:512:28)
    at Pipe.<anonymous> (node:net:334:12) {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: Error: Command failed: cat /Users/yagiz/Developer/node/test/.tmp.0/readfilesync_pipe_large_test.txt | "/Users/yagiz/Developer/node/out/Release/node" "/Users/yagiz/Developer/node/test/parallel/test-fs-readfilesync-pipe-large.js" child
  node:internal/fs/utils:351
      throw err;
      ^

  Error: ESPIPE: invalid seek, undefined
      at readFileSyncUtf8 (node:internal/fs/read-file/utf8:19:3)
      at Object.readFileSync (node:fs:467:12)
      at Object.<anonymous> (/Users/yagiz/Developer/node/test/parallel/test-fs-readfilesync-pipe-large.js:14:27)
      at Module._compile (node:internal/modules/cjs/loader:1233:14)
      at Module._extensions..js (node:internal/modules/cjs/loader:1287:10)
      at Module.load (node:internal/modules/cjs/loader:1091:32)
      at Module._load (node:internal/modules/cjs/loader:938:12)
      at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:83:12)
      at node:internal/main/run_main_module:23:47 {
    errno: -29,
    code: 'ESPIPE'
  }

  Node.js v21.0.0-pre

      at ChildProcess.exithandler (node:child_process:419:12)
      at ChildProcess.emit (node:events:512:28)
      at maybeClose (node:internal/child_process:1105:16)
      at Socket.<anonymous> (node:internal/child_process:457:11)
      at Socket.emit (node:events:512:28)
      at Pipe.<anonymous> (node:net:334:12) {
    code: 1,
    killed: false,
    signal: null,
    cmd: 'cat /Users/yagiz/Developer/node/test/.tmp.0/readfilesync_pipe_large_test.txt | "/Users/yagiz/Developer/node/out/Release/node" "/Users/yagiz/Developer/node/test/parallel/test-fs-readfilesync-pipe-large.js" child'
  },
  expected: null,
  operator: 'ifError'
}

Node.js v21.0.0-pre

@mcollina
Copy link
Member

mcollina commented Jul 5, 2023

great work!

@anonrig anonrig marked this pull request as ready for review July 5, 2023 19:55
src/node_file.cc Outdated Show resolved Hide resolved
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.

lgtm

I would recommend a CITGM run anyway

@anonrig anonrig added needs-citgm PRs that need a CITGM CI run. request-ci Add this label to start a Jenkins CI on a PR. labels Jul 6, 2023
@anonrig
Copy link
Member Author

anonrig commented Jul 6, 2023

Is this PR worth having a notable-change label? I believe informing & sharing with the users might be a good thing. Please, remove it, if you think it's unnecessary. cc @nodejs/performance

@anonrig anonrig added the notable-change PRs with changes that should be highlighted in changelogs. label Jul 6, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2023

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @anonrig.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 6, 2023
@nodejs-github-bot
Copy link
Collaborator

@ruyadorno ruyadorno added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Sep 12, 2023
H4ad pushed a commit to H4ad/node that referenced this pull request Sep 17, 2023
H4ad pushed a commit to H4ad/node that referenced this pull request Sep 17, 2023
H4ad pushed a commit to H4ad/node that referenced this pull request Sep 17, 2023
H4ad added a commit to H4ad/node that referenced this pull request Sep 17, 2023
H4ad pushed a commit to H4ad/node that referenced this pull request Sep 17, 2023
H4ad pushed a commit to H4ad/node that referenced this pull request Sep 17, 2023
H4ad pushed a commit to H4ad/node that referenced this pull request Sep 17, 2023
H4ad pushed a commit to H4ad/node that referenced this pull request Sep 17, 2023
H4ad added a commit to H4ad/node that referenced this pull request Sep 17, 2023
H4ad pushed a commit to H4ad/node that referenced this pull request Sep 17, 2023
H4ad pushed a commit to H4ad/node that referenced this pull request Sep 17, 2023
H4ad pushed a commit to H4ad/node that referenced this pull request Sep 17, 2023
H4ad pushed a commit to H4ad/node that referenced this pull request Sep 17, 2023
H4ad added a commit to H4ad/node that referenced this pull request Sep 17, 2023
H4ad pushed a commit to H4ad/node that referenced this pull request Sep 17, 2023
H4ad pushed a commit to H4ad/node that referenced this pull request Sep 17, 2023
H4ad pushed a commit to H4ad/node that referenced this pull request Sep 17, 2023
H4ad added a commit to H4ad/node that referenced this pull request Dec 2, 2023
H4ad pushed a commit to H4ad/node that referenced this pull request Dec 2, 2023
H4ad pushed a commit to H4ad/node that referenced this pull request Dec 2, 2023
H4ad pushed a commit to H4ad/node that referenced this pull request Dec 2, 2023
H4ad pushed a commit to H4ad/node that referenced this pull request Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. needs-citgm PRs that need a CITGM CI run. notable-change PRs with changes that should be highlighted in changelogs. 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