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.

Sign up for GitHub

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

Jump to bottom

fs: improve mkdtemp performance for buffer prefix #51078

Merged
merged 1 commit into from
Dec 20, 2023
Merged

fs: improve mkdtemp performance for buffer prefix #51078

merged 1 commit into from
Dec 20, 2023

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Dec 6, 2023 •
edited
Loading

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 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 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 anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 6, 2023
@github-actions 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
Copy link
Collaborator

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

@anonrig
Copy link
Member Author

anonrig commented Dec 7, 2023

cc @nodejs/performance appreciate any reviews!

@anonrig anonrig added the review wanted PRs that need reviews. label Dec 7, 2023
MarshallOfSound
MarshallOfSound reviewed Dec 7, 2023
View reviewed changes
src/node_file.cc Outdated Show resolved Hide resolved
H4ad
H4ad approved these changes Dec 7, 2023
View reviewed changes
Copy link
Member

@H4ad H4ad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with some minor comments.

benchmark/fs/bench-mkdtempSync.js Outdated Show resolved Hide resolved
benchmark/fs/bench-mkdtempSync.js Outdated Show resolved Hide resolved
mcollina
mcollina approved these changes Dec 7, 2023
View reviewed changes
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

allthingsasync
allthingsasync reviewed Dec 8, 2023
View reviewed changes
src/node_file.cc Outdated Show resolved Hide resolved
jasnell
jasnell reviewed Dec 9, 2023
View reviewed changes
src/node_file.cc Outdated Show resolved Hide resolved
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 15, 2023
@github-actions 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
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 20, 2023
@nodejs-github-bot 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 nodejs-github-bot merged commit 202c872 into nodejs:main Dec 20, 2023
54 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 202c872

RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024
@anonrig @RafaelGSS
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 RafaelGSS mentioned this pull request Jan 2, 2024
Merged
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@anonrig @richardlau
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 richardlau mentioned this pull request Mar 25, 2024
Merged
@YieldRay YieldRay mentioned this pull request Jul 9, 2024
Merged
nodejs-github-bot pushed a commit that referenced this pull request Jul 23, 2024
@YieldRay
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
@YieldRay @targos
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
@YieldRay @RafaelGSS
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
@YieldRay @marco-ippolito
fs: ensure consistency for mkdtemp in both fs and fs/promises
076f87d 
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
@YieldRay @marco-ippolito
fs: ensure consistency for mkdtemp in both fs and fs/promises
fa3ecee 
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
@YieldRay @marco-ippolito
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@allthingsasync allthingsasync allthingsasync left review comments

@MarshallOfSound MarshallOfSound MarshallOfSound left review comments

@targos targos targos left review comments

@jasnell jasnell jasnell left review comments

@H4ad H4ad H4ad approved these changes

@mcollina mcollina mcollina approved these changes

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
@anonrig @nodejs-github-bot @mcollina @jasnell @targos @MarshallOfSound @H4ad @allthingsasync