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 error performance for fs.mkdtempSync #49750

Closed
wants to merge 2 commits into from
Closed

fs: improve error performance for fs.mkdtempSync #49750

wants to merge 2 commits into from

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Sep 21, 2023 

                                               confidence improvement accuracy (*)    (**)   (***)
fs/bench-mkdtempSync.js n=1000 type='invalid'        ***     78.55 %      ±18.94% ±25.20% ±32.82%
fs/bench-mkdtempSync.js n=1000 type='valid'                  -0.72 %       ±7.92% ±10.54% ±13.72%

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

Ref: nodejs/performance#106

@nodejs-github-bot nodejs-github-bot added 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. labels Sep 21, 2023
@anonrig anonrig mentioned this pull request Sep 21, 2023
Open
@anonrig anonrig added performance Issues and PRs related to the performance of Node.js. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 21, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Sep 22, 2023
Open
17 tasks
This was referenced Sep 23, 2023
Open
Open
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@Trott
Copy link
Member

Trott commented Sep 25, 2023

@nodejs/fs @nodejs/cpp-reviewers

@anonrig anonrig requested a review from Qard September 26, 2023 15:11
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 26, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 26, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@JakobJingleheimer
Copy link
Member

For us luddites, I would love a TLDR for why this improved perf so drastically 😅

@anonrig
Copy link
Member Author

anonrig commented Sep 26, 2023

For us luddites, I would love a TLDR for why this improved perf so drastically 😅

Because the cost of throwing an error on C++ vs. the cost of throwing an error on JS is extremely different. We do lots of stuff on JavaScript land whenever we create an error. For example, hiding stack traces etc. On the other hand, C++ has this support out of the box.

@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Sep 27, 2023
Open
21 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

benjamingr
benjamingr reviewed Sep 27, 2023
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Left one comment other than that LGTM, after fixing feel free to consider my review as a LGTM

src/node_file.cc Outdated
@@ -3409,6 +3441,7 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data,
SetMethod(isolate, target, "lutimes", LUTimes);

SetMethod(isolate, target, "mkdtemp", Mkdtemp);
SetMethodNoSideEffect(isolate, target, "mkdtempSync", MkdtempSync);
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be SetMethod right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes...

anonrig
anonrig commented Sep 27, 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 Sep 27, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 27, 2023
@nodejs-github-bot
Copy link
Collaborator

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

joyeecheung
joyeecheung reviewed Sep 27, 2023
View reviewed changes
benchmark/fs/bench-mkdtempSync.js

switch (type) {
case 'valid':
prefix = `${Date.now()}`;
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be wrapped with tmpdir.resolve(..)

benchmark/fs/bench-mkdtempSync.js
for (let i = 0; i < n; i++) {
try {
const folderPath = fs.mkdtempSync(prefix, { encoding: 'utf8' });
fs.rmdirSync(folderPath);
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to remove it here? I think as long as n does not exceed the number of combinations formed by the 6 random characters it should be fine to just leave them in the loop and let test common just delete the tmpdir on exit.

lib/internal/fs/sync.js
@@ -53,6 +56,25 @@ function copyFile(src, dest, mode) {
);
}

function mkdtemp(prefix, options) {
options = getOptions(options);
Copy link
Member

Choose a reason for hiding this comment

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

hmm, not sure since we we started to move the sync implementations to a different file, but I gotta say I am not a fan because 1) this incurs an unnecessary hit to startup (even with deserialization) and 2) moving code around makes git blame less useful, especially when it's useful to have the sync and the async version side by side for cross-reference..

src/node_file.cc
@@ -2957,6 +2957,38 @@ static void Mkdtemp(const FunctionCallbackInfo<Value>& args) {
}
}

static void MkdtempSync(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Copy link
Member

Choose a reason for hiding this comment

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

hmm, actually, why do we need a different binding? We could just change the sync branch of the original implementation to directly throw instead of passing the error down to ctx. This would avoid the repetition of most of the code here. We can probably tell if it's a sync call or not by checking the length of the arguments now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I personally find it a lot harder to optimize since the requirements for promise implementation, sync and callback implementations are connected a lot.

Copy link
Member

@joyeecheung joyeecheung Sep 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Why would it be a lot harder to optimize? We just need to introduce another SyncCall helper that throws instead of setting the ctx and use that in the original implementation, and it's essentially the same as what's being done here? Then that helper can be used across the board for similar optmizations.

@github-actions github-actions bot mentioned this pull request Sep 28, 2023
Open
19 tasks
@anonrig anonrig mentioned this pull request Sep 29, 2023
Closed
@anonrig
Copy link
Member Author

anonrig commented Sep 29, 2023

Closing in favor of #49962

@anonrig anonrig closed this Sep 29, 2023
@github-actions github-actions bot mentioned this pull request Sep 30, 2023
Open
31 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung left review comments

@benjamingr benjamingr benjamingr left review comments

@rluvaton rluvaton Awaiting requested review from rluvaton

@Qard Qard Awaiting requested review from Qard

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@anonrig @nodejs-github-bot @Trott @JakobJingleheimer @benjamingr @joyeecheung @rluvaton