`node:fs` error path performance improvements by anonrig · Pull Request #49962 · nodejs/node

anonrig · 2023-09-29T17:20:04Z

Local benchmarks:

readlinkSync, linkSync, symlinkSync

fs/bench-linkSync.js n=1000 type='invalid' *** 33.31 % ±1.38% ±1.84% ±2.40%
fs/bench-linkSync.js n=1000 type='valid' -1.06 % ±3.29% ±4.40% ±5.77%
fs/bench-readlinkSync.js n=1000 type='invalid' *** 25.55 % ±0.96% ±1.28% ±1.66%
fs/bench-readlinkSync.js n=1000 type='valid' 4.01 % ±6.24% ±8.32% ±10.87%
fs/bench-symlinkSync.js n=1000 type='invalid' *** 31.08 % ±1.08% ±1.45% ±1.90%
fs/bench-symlinkSync.js n=1000 type='valid' 1.37 % ±3.34% ±4.45% ±5.79%

renameSync

fs/bench-renameSync.js n=2000 type='invalid' *** 44.88 % ±4.54% ±6.06% ±7.93%
fs/bench-renameSync.js n=2000 type='valid' -1.36 % ±4.57% ±6.08% ±7.92%

chownSync & lchownSync

fs/bench-chownSync.js n=10000 method='chownSync' type='existing' ** 3.08 % ±1.93% ±2.58% ±3.39%
fs/bench-chownSync.js n=10000 method='chownSync' type='non-existing' *** 90.16 % ±1.57% ±2.09% ±2.72%
fs/bench-chownSync.js n=10000 method='lchownSync' type='existing' -0.03 % ±1.99% ±2.65% ±3.45%
fs/bench-chownSync.js n=10000 method='lchownSync' type='non-existing' *** 87.50 % ±1.75% ±2.33% ±3.06%

mkdtempSync

fs/bench-mkdtempSync.js n=1000 type='invalid' *** 54.66 % ±1.72% ±2.30% ±3.01%
fs/bench-mkdtempSync.js n=1000 type='valid' -1.20 % ±3.58% ±4.77% ±6.20%

Ref: nodejs/performance#106

cc @nodejs/performance

nodejs-github-bot · 2023-09-29T17:20:09Z

Review requested:

@nodejs/startup

CanadaHonk

LGTM from initial pass

joyeecheung · 2023-09-30T16:21:50Z

This needs a rebase since #49913 landed

nodejs-github-bot · 2023-09-30T19:05:09Z

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

nodejs-github-bot · 2023-09-30T21:48:20Z

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

anonrig · 2023-09-30T22:12:52Z

Errors seems to be related. I'll look into it.

nodejs-github-bot · 2023-10-01T00:32:24Z

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

nodejs-github-bot · 2023-10-18T02:27:47Z

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

StefanStojanovic · 2023-10-18T08:37:03Z

The test that's problematic on Windows in the last few runs (parallel/test-worker-nearheaplimit-deadlock) is flaky, but only on x86. I've tested it locally on main to confirm that. The CI could be rerun until it eventually passes, but regardless of that, I plan to open a PR marking the flaky test on x86 Windows.

nodejs-github-bot · 2023-10-18T11:44:59Z

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

nodejs-github-bot · 2023-10-18T13:58:54Z

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

anonrig · 2023-10-18T15:14:45Z

We finally got the green CI. Appreciate it if anybody can leave a new review, so we can land this!

The test is only flaky on x86 Windows. Fixes: #50220 PR-URL: #50238 Refs: #49962 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Michael Dawson <midawson@redhat.com>

nodejs-github-bot · 2023-10-18T15:56:29Z

Landed in dbd0ffa...1a5db6f

PR-URL: #49962 Refs: nodejs/performance#106 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>

The test is only flaky on x86 Windows. Fixes: #50220 PR-URL: #50238 Refs: #49962 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Michael Dawson <midawson@redhat.com>

PR-URL: #49962 Refs: nodejs/performance#106 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>

anonrig added performance Issues and PRs related to the performance of Node.js. needs-benchmark-ci PR that need a benchmark CI run. labels Sep 29, 2023

anonrig requested a review from joyeecheung September 29, 2023 17:20

anonrig changed the title ~~Fs improvements~~node:fs error path performance improvementsSep 29, 2023

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 Sep 29, 2023

anonrig mentioned this pull request Sep 29, 2023
fs: improve error performance for fs.renameSync#49863
Closed

anonrig added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Sep 29, 2023

This was referenced Sep 29, 2023
fs: improve error performance for fs.mkdtempSync#49750
Closed
fs: improve error performance of chownSync#49748
Closed
Performance of node:fsnodejs/performance#106
Open

CanadaHonk approved these changes Sep 29, 2023
View reviewed changes

anonrig force-pushed the fs-improvements branch from 88b3339 to 8c81eadCompare September 30, 2023 18:56

anonrig marked this pull request as ready for review September 30, 2023 18:56

anonrig force-pushed the fs-improvements branch from 8c81ead to 842b986Compare September 30, 2023 18:57

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2023

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2023

github-actionsBot mentioned this pull request Oct 1, 2023
CI Reliability 2023-10-01 nodejs/reliability#680
Open
16 tasks

anonrig force-pushed the fs-improvements branch from da5fcd9 to faf02c5Compare October 1, 2023 00:19

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 1, 2023

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 1, 2023

anonrig force-pushed the fs-improvements branch 2 times, most recently from 573b0bf to c437b20Compare October 1, 2023 02:27

StefanStojanovic mentioned this pull request Oct 18, 2023
test: set test-worker-nearheaplimit-deadlock flaky #50238
Merged

H4ad approved these changes Oct 18, 2023
View reviewed changes

H4ad added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 18, 2023

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 18, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 18, 2023

nodejs-github-bot closed this Oct 18, 2023

This was referenced Oct 19, 2023
CI Reliability 2023-10-19 nodejs/reliability#691
Open
CI Reliability 2023-10-20 nodejs/reliability#692
Open

targos mentioned this pull request Oct 23, 2023
v21.1.0 release proposal #50335
Merged

targos mentioned this pull request Nov 12, 2023
v20.10.0 release proposal #50682
Merged

juanarbol mentioned this pull request Dec 6, 2023
2023-12-06, N|Solid v5.0.1 Iron Release nodesource/nsolid#41
Merged

Uh oh!

Uh oh!

Conversation

anonrig commented Sep 29, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Local benchmarks:

Uh oh!

nodejs-github-bot commented Sep 29, 2023

Uh oh!

CanadaHonk left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Sep 30, 2023

Uh oh!

nodejs-github-bot commented Sep 30, 2023

Uh oh!

nodejs-github-bot commented Sep 30, 2023

Uh oh!

anonrig commented Sep 30, 2023

Uh oh!

nodejs-github-bot commented Oct 1, 2023

Uh oh!

nodejs-github-bot commented Oct 18, 2023

Uh oh!

StefanStojanovic commented Oct 18, 2023

Uh oh!

nodejs-github-bot commented Oct 18, 2023

Uh oh!

nodejs-github-bot commented Oct 18, 2023

Uh oh!

anonrig commented Oct 18, 2023

Uh oh!

nodejs-github-bot commented Oct 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

anonrig commented Sep 29, 2023•
edited
Loading