fs: improve error performance of opendirSync

[anonrig]

fs/bench-opendirSync.js n=10000 bufferSize=128 type='existing'                    -0.15 %       ±1.56%  ±2.08%  ±2.71%
fs/bench-opendirSync.js n=10000 bufferSize=128 type='non-existing'        ***    109.56 %       ±3.93%  ±5.23%  ±6.81%
fs/bench-opendirSync.js n=10000 bufferSize=32 type='existing'                     -0.34 %       ±1.95%  ±2.60%  ±3.39%
fs/bench-opendirSync.js n=10000 bufferSize=32 type='non-existing'         ***    111.68 %       ±3.03%  ±4.03%  ±5.25%
fs/bench-opendirSync.js n=10000 bufferSize=4 type='existing'                      -1.19 %       ±1.44%  ±1.92%  ±2.51%
fs/bench-opendirSync.js n=10000 bufferSize=4 type='non-existing'          ***    104.85 %       ±8.18% ±11.01% ±14.59%

Ref: nodejs/performance#106

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[BridgeAR]

Could we improve error handling in JS in general before we move all of this into C++? I believe there's likely not a big difference anymore as soon as that's fixed.

[anonrig]

Could we improve error handling in JS in general before we move all of this into C++? I believe there's likely not a big difference anymore as soon as that's fixed.

@BridgeAR There is also the cost of crossing the C++ and JavaScript boundary that'll still be there even after we improve the error creation performance.

[nodejs-github-bot]

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

[anonrig]

cc @nodejs/cpp-reviewers can you review?

[joyeecheung]

Could we improve error handling in JS in general before we move all of this into C++?

For the simple FS errors I think it would probably be pretty hard to make the JS error creation performance on-par with the C++ one if we want to hide internal frames. If we throw from C++, we hide them for free, the stack is only going to be captured when the user access error.stack, if they don't need it there's no cost paid. If we have to throw from JS, we need to capture the stack trace eagerly to hide internal frames. And not having to capture the stack trace is always going to be faster than having to do that.

[targos]

Shouldn't openDir (the C++ one) be also updated to remove the sync code now that it's in another function?

[anonrig]

Shouldn't openDir (the C++ one) be also updated to remove the sync code now that it's in another function?

@targos OpenDir is still used with callback implementation. After working on Sync functions, I'll move to callback implementations and make OpenDir like functions to only handle promise implementations.

[nodejs-github-bot]

Landed in 571ecbf

[RafaelGSS]

I see the needs-benchmark-ci label, but I don't see any benchmark ci. Just a possible local benchmarks in the PR description.

[anonrig]

@RafaelGSS I didn't follow up with the label I've added. I don't think it's necessary for this pull request since it's highly unlikely to cause regression.

[RafaelGSS]

But would be great to have actual reliable numbers to share. People might see this PR when scrolling the CHANGELOG.