Skip to content

fs: add a fast-path for readFileSync utf-8#48658

Merged
nodejs-github-bot merged 5 commits into
nodejs:mainfrom
anonrig:node-file
Jul 12, 2023
Merged

fs: add a fast-path for readFileSync utf-8#48658
nodejs-github-bot merged 5 commits into
nodejs:mainfrom
anonrig:node-file

Conversation

@anonrig

@anonrig anonrig commented Jul 5, 2023

Copy link
Copy Markdown
Member

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
Comment thread lib/fs.js Outdated
@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

anonrig commented Jul 5, 2023

Copy link
Copy Markdown
Member Author

@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

mcollina commented Jul 5, 2023

Copy link
Copy Markdown
Member

great work!

@anonrig anonrig marked this pull request as ready for review July 5, 2023 19:55
Comment thread src/node_file.cc Outdated

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

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

anonrig commented Jul 6, 2023

Copy link
Copy Markdown
Member Author

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

github-actions Bot commented Jul 6, 2023

Copy link
Copy Markdown
Contributor

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

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.

7 participants