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

fs: improve mkdtemp performance for buffer prefix by anonrig · Pull Request #51078 · nodejs/node · GitHub

/ node Public

fs: improve mkdtemp performance for buffer prefix - #51078

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
anonrig:improve-mkdtemp
Dec 20, 2023
ConversationCommits (1)ChecksFiles changed
Merged

fs: improve mkdtemp performance for buffer prefix#51078
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
anonrig:improve-mkdtemp

Conversation

anonrig commented Dec 6, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

This small PR improves the mkdtempSync performance for buffer inputs by 5%. Basically, we achieve this performance boost because Buffer.concat() is costly, and doing it in C++ is beneficial for performance.

Local benchmarks

❯ node benchmark/compare.js --old ./node-main --new out/Release/node --filter mkdtempSync fs > fs.csv && node-benchmark-compare fs.csv
[00:00:25|% 100| 1/1 files | 60/60 runs | 3/3 configs]: Done
                                                    confidence improvement accuracy (*)   (**)   (***)
fs/bench-mkdtempSync.js n=1000 type='invalid'                      1.64 %       ±6.96% ±9.28% ±12.11%
fs/bench-mkdtempSync.js n=1000 type='valid-buffer'        ***      5.52 %       ±2.97% ±3.95%  ±5.14%
fs/bench-mkdtempSync.js n=1000 type='valid-string'                -1.40 %       ±3.73% ±4.96%  ±6.46%

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

Benchmark CI

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

                                                   confidence improvement accuracy (*)   (**)  (***)
fs/bench-mkdtempSync.js n=1000 type='invalid'                     -2.32 %       ±2.97% ±3.97% ±5.19%
fs/bench-mkdtempSync.js n=1000 type='valid-buffer'        ***     21.88 %       ±2.87% ±3.84% ±5.02%
fs/bench-mkdtempSync.js n=1000 type='valid-string'                -1.53 %       ±4.60% ±6.13% ±8.00%

anonrig added fs Issues and PRs related to the fs subsystem / file system. performance Issues and PRs related to the performance of Node.js. labels Dec 6, 2023
nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Dec 6, 2023
anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 6, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 6, 2023

nodejs-github-bot commented Dec 6, 2023

Copy link
Copy Markdown
Collaborator

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

anonrig commented Dec 7, 2023

Copy link
Copy Markdown
Member Author

cc @nodejs/performance appreciate any reviews!

anonrig added the review wanted PRs that need reviews. label Dec 7, 2023
MarshallOfSound reviewed Dec 7, 2023
View reviewed changes
Comment thread src/node_file.cc Outdated
H4ad approved these changes Dec 7, 2023
View reviewed changes

H4ad 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, with some minor comments.

Comment thread benchmark/fs/bench-mkdtempSync.js Outdated
Comment thread benchmark/fs/bench-mkdtempSync.js Outdated
mcollina approved these changes Dec 7, 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

allthingsasync reviewed Dec 8, 2023
View reviewed changes
Comment thread src/node_file.cc Outdated
jasnell reviewed Dec 9, 2023
View reviewed changes
Comment thread src/node_file.cc Outdated
anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 15, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 15, 2023

nodejs-github-bot commented Dec 15, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Dec 15, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Dec 19, 2023

Copy link
Copy Markdown
Collaborator

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

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 20, 2023
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 20, 2023
nodejs-github-bot merged commit 202c872 into nodejs:main Dec 20, 2023

nodejs-github-bot commented Dec 20, 2023

Copy link
Copy Markdown
Collaborator

Landed in 202c872

RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024
fs: improve mkdtemp performance for buffer prefix
7421467 
PR-URL: #51078
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
RafaelGSS mentioned this pull request Jan 2, 2024
Merged
richardlau pushed a commit that referenced this pull request Mar 25, 2024
fs: improve mkdtemp performance for buffer prefix
65df2c6 
PR-URL: #51078
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
richardlau mentioned this pull request Mar 25, 2024
Merged
YieldRay mentioned this pull request Jul 9, 2024
Merged
nodejs-github-bot pushed a commit that referenced this pull request Jul 23, 2024
fs: ensure consistency for mkdtemp in both fs and fs/promises
42fd640 
Port changes for mkdtemp from lib/fs.js to lib/internal/fs/promise.js,
ensuring consistent behavior.

Refs: #51078
PR-URL: #53776
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
targos pushed a commit that referenced this pull request Jul 28, 2024
fs: ensure consistency for mkdtemp in both fs and fs/promises
1cb0363 
Port changes for mkdtemp from lib/fs.js to lib/internal/fs/promise.js,
ensuring consistent behavior.

Refs: #51078
PR-URL: #53776
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
RafaelGSS pushed a commit that referenced this pull request Aug 5, 2024
fs: ensure consistency for mkdtemp in both fs and fs/promises
e0054ee 
Port changes for mkdtemp from lib/fs.js to lib/internal/fs/promise.js,
ensuring consistent behavior.

Refs: #51078
PR-URL: #53776
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
marco-ippolito pushed a commit that referenced this pull request Aug 19, 2024
fs: ensure consistency for mkdtemp in both fs and fs/promises
490f15a 
Port changes for mkdtemp from lib/fs.js to lib/internal/fs/promise.js,
ensuring consistent behavior.

Refs: #51078
PR-URL: #53776
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
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

jasnell jasnell left review comments
mcollina mcollina approved these changes
H4ad H4ad approved these changes
+3 more reviewers
targos targos left review comments
MarshallOfSound MarshallOfSound left review comments
allthingsasync allthingsasync left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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. needs-ci PRs that need a full CI run. performance Issues and PRs related to the performance of Node.js. review wanted PRs that need reviews.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL