fs: use fast api calls for `existsSync` by littledivy · Pull Request #49893 · nodejs/node

littledivy · 2023-09-27T12:09:52Z

Currently, It takes the fast route when path string is represented as a OneByteString in V8.

 confidence improvement accuracy (*) (**) (***)
fs/bench-existsSync.js n=1000000 type='existing' *** 2.87 % ±0.68% ±0.91% ±1.19%
fs/bench-existsSync.js n=1000000 type='non-existing' *** 43.04 % ±1.17% ±1.56% ±2.03%
fs/bench-existsSync.js n=1000000 type='non-flat-existing' *** 2.63 % ±0.42% ±0.56% ±0.73%
n=1000000 type='non-existing' 836103.841448331 NA NaN % NA NA NA
Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 4 comparisons, you can thus expect the following amount of false-positive results:
0.20 false positives, when considering a 5% risk acceptance (*, **, ***),
0.04 false positives, when considering a 1% risk acceptance (**, ***),
0.00 false positives, when considering a 0.1% risk acceptance (***)

nodejs-github-bot · 2023-09-27T12:44:24Z

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

anonrig · 2023-09-27T13:16:23Z

Can you resolve the conflict @littledivy?

debadree25

Guess we have to add a test like in test/parallel/test-url-canParse-whatwg.js ?

nodejs-github-bot · 2023-09-27T17:05:41Z

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

nodejs-github-bot · 2023-09-27T17:13:55Z

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

joyeecheung · 2023-09-27T22:59:42Z

+ uv_fs_t req;
+ auto make = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); });
+ FS_SYNC_TRACE_BEGIN(access);
+ int err = uv_fs_access(nullptr, &req, path.out(), 0, nullptr);
+ FS_SYNC_TRACE_END(access);
+
+#ifdef _WIN32
+ // In case of an invalid symlink, `uv_fs_access` on win32
+ // will **not** return an error and is therefore not enough.
+ // Double check with `uv_fs_stat()`.
+ if (err == 0) {
+ FS_SYNC_TRACE_BEGIN(stat);
+ err = uv_fs_stat(nullptr, &req, path.out(), nullptr);
+ FS_SYNC_TRACE_END(stat);
+ }
+#endif // _WIN32


The fs operation part can be wrapped in a helper and shared with the slow callback to avoid getting out of sync.

joyeecheung · 2023-09-27T23:10:55Z

+ // This test is to ensure that the v8 fast api works.
+ const oneBytePath = 'hello.txt';
+ for (let i = 0; i < 1e5; i++) {
+ assert(!fs.existsSync(oneBytePath));


This should be moved to test/pummel instead. But also, in general we need to avoid running tight loops in the tests to avoid introducing timeouts in the CI on the slower machines. Maybe it's already enough that the fast path is exercised in the benchmark..

Can't we use V8 natives API like they do unit test fast calls? IIRC you need to %PrepareForOptimization(fn), call the function, %OptimizeOnNextCall(fn), and call it again. That last call should be optimized.

Also, while I'm not good enough with C++ to suggest how to implement it, I think we can put something into place (maybe only in debug builds?). For example, the fast version, when called, increases some counter that we can get from JavaScript for an assertion.

I think we can keep an array of booleans for all fast APIs to see if they are called, and expose them to the JS land, toggling a boolean shouldn't be very expensive.

For this PR though doing %PrepareForOptimization() and %OptimizeOnNextCall() in the tests may be fine. But we also need to check in JS land if the optimizing compiler is enabled at all in the test to avoid failing on builds that turns optimizations off, which would be tricky..

Using `await fs.access` has couple of downsides. It creates unnecessary async contexts where async scope can be removed. Also, it creates the possibility of race conditions such as `Time-of-Check to Time-of-Use`. It would be nice if someone can benchmark this. I'm rooting for a performance improvement. Some updates from Node.js land: - There is an open pull request to add V8 Fast API to `existsSync` method - nodejs/node#49893 - Non-existing `existsSync` executions became 30% faster - nodejs/node#49593 Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>

aduh95 · 2024-05-11T14:37:00Z

This needs a rebase.

avivkeller · 2024-08-09T22:11:15Z

👋 Hey, this has approvals and a decent number of reviews, but currently has conflcits with the main branch. @aduh95 pointed out a few months ago that this needs a rebase, but no action has been taken since then.

I've marked this PR as stalled for these reasons, LMK if this wasn't the right thing to do, and feel free to undo it :-).

github-actions · 2024-08-09T22:11:30Z

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

littledivy · 2024-10-15T12:51:24Z

Sorry, I haven't had a chance to rebase. Please feel free to take over and get this landed.

avivkeller · 2024-10-15T16:50:15Z

You can always re-open, the stalled bot isn't working, so I was just doing some maintenance

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 27, 2023

littledivy added 2 commits September 27, 2023 17:46

fs: improve performance of existsSync
f413971

Format
dd08af9

littledivy force-pushed the perf-existsSync branch from e5014d5 to dd08af9Compare September 27, 2023 12:17

panva 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 27, 2023

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

anonrig approved these changes Sep 27, 2023
View reviewed changes

anonrig added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 27, 2023

debadree25 reviewed Sep 27, 2023
View reviewed changes

anonrig added the needs-benchmark-ci PR that need a benchmark CI run. label Sep 27, 2023

mscdex reviewed Sep 27, 2023
View reviewed changes

Comment threadsrc/node_file.cc

anonrig removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 27, 2023

Merge branch 'main' into perf-existsSync
ac4be44

anonrig added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Sep 27, 2023

littledivy added 2 commits September 27, 2023 19:33

Add test for fast call
69736c2

Fix lint js
176cbdc

anonrig reviewed Sep 27, 2023
View reviewed changes

Comment threadtest/parallel/test-fs-exists.js

Add test for true existsSync
8d331ed

CanadaHonk reviewed Sep 27, 2023
View reviewed changes

Comment threadsrc/node_file.cc Outdated

tniessen reviewed Sep 27, 2023
View reviewed changes

Comment threadsrc/node_file.cc Outdated

littledivy added 2 commits September 27, 2023 21:14

Use SetFastMethod
7a37e48

Format
e2e7341

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

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

tniessen approved these changes Sep 27, 2023
View reviewed changes

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 reviewed Sep 29, 2023
View reviewed changes

Comment threadsrc/node_file.cc

anonrig mentioned this pull request Oct 3, 2023
fix: use fs.existsSync to avoid race condition vercel/next.js#56387
Merged

fhinkel approved these changes Jan 28, 2024
View reviewed changes

avivkeller added the stalled Issues and PRs that are stalled. label Aug 9, 2024

avivkeller closed this Oct 15, 2024

Uh oh!

Uh oh!

Conversation

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

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 27, 2023

Uh oh!

anonrig commented Sep 27, 2023

Uh oh!

debadree25 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 27, 2023

Uh oh!

nodejs-github-bot commented Sep 27, 2023

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!

joyeecheungSep 27, 2023

Choose a reason for hiding this comment

Uh oh!

targosSep 28, 2023

Choose a reason for hiding this comment

Uh oh!

targosSep 29, 2023

Choose a reason for hiding this comment

Uh oh!

joyeecheungOct 2, 2023

Choose a reason for hiding this comment

Uh oh!

joyeecheungOct 2, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aduh95 commented May 11, 2024

Uh oh!

avivkeller commented Aug 9, 2024

Uh oh!

github-actionsBot commented Aug 9, 2024

Uh oh!

littledivy commented Oct 15, 2024

Uh oh!

avivkeller commented Oct 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

littledivy commented Sep 27, 2023•
edited
Loading

joyeecheungSep 27, 2023•
edited
Loading