FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fs: add a fast-path for readFileSync utf-8 by anonrig · Pull Request #48658 · nodejs/node · GitHub

/ node Public

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
ConversationCommits (5)ChecksFiles changed
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 commented Jul 5, 2023 •
edited
Loading

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 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
anonrig commented Jul 5, 2023
View reviewed changes
Comment thread lib/fs.js Outdated
anonrig force-pushed the node-file branch 3 times, most recently from 3ecaadc to 1227128 Compare July 5, 2023 16:12
anonrig added the performance Issues and PRs related to the performance of Node.js. label Jul 5, 2023

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 commented Jul 5, 2023

Copy link
Copy Markdown
Member

great work!

anonrig marked this pull request as ready for review July 5, 2023 19:55
santigimeno reviewed Jul 5, 2023
View reviewed changes
Comment thread src/node_file.cc Outdated
mcollina approved these changes Jul 6, 2023
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

I would recommend a CITGM run anyway

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 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 added the notable-change PRs with changes that should be highlighted in changelogs. label Jul 6, 2023

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 Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 6, 2023

nodejs-github-bot commented Jul 6, 2023

Copy link
Copy Markdown
Collaborator

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

H4ad mentioned this pull request Sep 18, 2023
Closed
CanadaHonk mentioned this pull request Sep 26, 2023
Open
sapphi-red mentioned this pull request Nov 7, 2023
Open
7 tasks
arcanis mentioned this pull request Nov 14, 2023
Open
sapphi-red mentioned this pull request Nov 20, 2023
Closed
9 tasks
regseb mentioned this pull request Nov 28, 2023
Open
GiladShoham mentioned this pull request Nov 29, 2023
Closed
H4ad added a commit to H4ad/node that referenced this pull request Dec 2, 2023
fs: add a fast-path for readFileSync utf-8
9cbe99b 
Backport-PR-URL: nodejs#48658
H4ad pushed a commit to H4ad/node that referenced this pull request Dec 2, 2023
fixup! fs: add a fast-path for readFileSync utf-8
3703195 
Backport-PR-URL: nodejs#48658
H4ad pushed a commit to H4ad/node that referenced this pull request Dec 2, 2023
fixup! fs: add a fast-path for readFileSync utf-8
a87fa4a 
Backport-PR-URL: nodejs#48658
H4ad pushed a commit to H4ad/node that referenced this pull request Dec 2, 2023
fixup! fs: add a fast-path for readFileSync utf-8
fdcc2da 
Backport-PR-URL: nodejs#48658
H4ad pushed a commit to H4ad/node that referenced this pull request Dec 2, 2023
fixup! fs: add a fast-path for readFileSync utf-8
cc7b552 
Backport-PR-URL: nodejs#48658
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

santigimeno santigimeno left review comments
mcollina mcollina approved these changes
benjamingr benjamingr approved these changes
addaleax Awaiting requested review from addaleax
jasnell Awaiting requested review from jasnell
bnoordhuis Awaiting requested review from bnoordhuis

Assignees

No one assigned

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL