fs: improve error performance for `fs.mkdtempSync`#49750

anonrig · 2023-09-21T17:14:32Z

 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 · 2023-09-21T17:26:53Z

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

nodejs-github-bot · 2023-09-21T19:42:35Z

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

benchmark/fs/bench-mkdtempSync.js

nodejs-github-bot · 2023-09-24T20:54:20Z

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

nodejs-github-bot · 2023-09-25T00:14:17Z

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

nodejs-github-bot · 2023-09-25T15:12:11Z

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

Trott · 2023-09-25T19:04:27Z

@nodejs/fs @nodejs/cpp-reviewers

nodejs-github-bot · 2023-09-26T15:43:53Z

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

JakobJingleheimer · 2023-09-26T18:46:16Z

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

anonrig · 2023-09-26T18:56:05Z

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 · 2023-09-26T22:16:21Z

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

nodejs-github-bot · 2023-09-27T13:15:15Z

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

nodejs-github-bot · 2023-09-27T15:06:24Z

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

benjamingr

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

benjamingr · 2023-09-27T17:09:36Z

src/node_file.cc

 SetMethod(isolate, target, "lutimes", LUTimes);

 SetMethod(isolate, target, "mkdtemp", Mkdtemp);
+SetMethodNoSideEffect(isolate, target, "mkdtempSync", MkdtempSync);


This needs to be SetMethod right?

src/node_file.cc

nodejs-github-bot · 2023-09-27T18:25:38Z

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

joyeecheung · 2023-09-27T20:29:49Z

benchmark/fs/bench-mkdtempSync.js

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


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

joyeecheung · 2023-09-27T20:34:02Z

benchmark/fs/bench-mkdtempSync.js

+for(leti=0;i<n;i++){
+try{
+constfolderPath=fs.mkdtempSync(prefix,{encoding: 'utf8'});
+fs.rmdirSync(folderPath);


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.

joyeecheung · 2023-09-27T20:39:00Z

lib/internal/fs/sync.js

 }

+functionmkdtemp(prefix,options){
+options=getOptions(options);


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..

joyeecheung · 2023-09-27T20:42:12Z

src/node_file.cc

 }

+staticvoidMkdtempSync(const FunctionCallbackInfo<Value>& args){
+ Environment* env = Environment::GetCurrent(args);


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.

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

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.

anonrig · 2023-09-29T17:21:32Z

Closing in favor of #49962

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 mentioned this pull request Sep 21, 2023
Performance of node:fsnodejs/performance#106
Open

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

github-actionsbot mentioned this pull request Sep 22, 2023
CI Reliability 2023-09-22 nodejs/reliability#673
Open
17 tasks

rluvaton reviewed Sep 22, 2023
View reviewed changes

benchmark/fs/bench-mkdtempSync.jsShow resolvedHide resolved

This was referenced Sep 23, 2023
CI Reliability 2023-09-23 nodejs/reliability#674
Open
CI Reliability 2023-09-24 nodejs/reliability#675
Open

anonrig requested a review from rluvaton September 24, 2023 20:47

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

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

anonrig force-pushed the fs-improve-mkdtempsync branch from e2ea13a to 476e8d4Compare September 24, 2023 21:10

anonrig requested a review from Qard September 26, 2023 15:11

fs: improve error performance for fs.mkdtempSync
58a4bcd

anonrig force-pushed the fs-improve-mkdtempsync branch from 476e8d4 to 58a4bcdCompare September 26, 2023 15:17

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

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

github-actionsbot mentioned this pull request Sep 27, 2023
CI Reliability 2023-09-27 nodejs/reliability#676
Open
21 tasks

benjamingr reviewed Sep 27, 2023
View reviewed changes

anonrig commented Sep 27, 2023
View reviewed changes

src/node_file.cc Outdated Show resolvedHide resolved

Update node_file.cc
ccb5830

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

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

joyeecheung reviewed Sep 27, 2023
View reviewed changes

github-actionsbot mentioned this pull request Sep 28, 2023
CI Reliability 2023-09-28 nodejs/reliability#677
Open
19 tasks

anonrig mentioned this pull request Sep 29, 2023
node:fs error path performance improvements #49962
Closed

anonrig closed this Sep 29, 2023

github-actionsbot mentioned this pull request Sep 30, 2023
CI Reliability 2023-09-30 nodejs/reliability#679
Open
31 tasks

Uh oh!

fs: improve error performance for fs.mkdtempSync#49750

fs: improve error performance for fs.mkdtempSync#49750

Uh oh!

Conversation

anonrig commented Sep 21, 2023

Uh oh!

nodejs-github-bot commented Sep 21, 2023

Uh oh!

nodejs-github-bot commented Sep 21, 2023

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 24, 2023

Uh oh!

nodejs-github-bot commented Sep 25, 2023

Uh oh!

nodejs-github-bot commented Sep 25, 2023

Uh oh!

Trott commented Sep 25, 2023

Uh oh!

nodejs-github-bot commented Sep 26, 2023

Uh oh!

JakobJingleheimer commented Sep 26, 2023

Uh oh!

anonrig commented Sep 26, 2023

Uh oh!

nodejs-github-bot commented Sep 26, 2023

Uh oh!

nodejs-github-bot commented Sep 27, 2023

Uh oh!

nodejs-github-bot commented Sep 27, 2023

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

benjamingrSep 27, 2023

Choose a reason for hiding this comment

Uh oh!

anonrigSep 27, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 27, 2023

Uh oh!

joyeecheungSep 27, 2023

Choose a reason for hiding this comment

Uh oh!

joyeecheungSep 27, 2023

Choose a reason for hiding this comment

Uh oh!

joyeecheungSep 27, 2023

Choose a reason for hiding this comment

Uh oh!

joyeecheungSep 27, 2023

Choose a reason for hiding this comment

Uh oh!

anonrigSep 27, 2023

Choose a reason for hiding this comment

Uh oh!

joyeecheungSep 27, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anonrig commented Sep 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fs: improve error performance for `fs.mkdtempSync`#49750

fs: improve error performance for `fs.mkdtempSync`#49750

joyeecheungSep 27, 2023•
edited
Loading