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

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 (***)

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

Can you resolve the conflict @littledivy?

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

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

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

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

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>

This needs a rebase.

👋 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 :-).

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.

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

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 dd08af9 Compare 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-actions Bot 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 outstanding review comments, 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 thread src/node_file.cc

anonrig removed the author ready PRs that have at least one approval, no outstanding review comments, 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 thread test/parallel/test-fs-exists.js

Add test for true existsSync

8d331ed

CanadaHonk reviewed Sep 27, 2023

View reviewed changes

Comment thread src/node_file.cc Outdated

tniessen reviewed Sep 27, 2023

View reviewed changes

Comment thread src/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-actions Bot 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-actions Bot 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-actions Bot 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 thread src/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

+                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

+                // 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));

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

Uh oh!

Uh oh!

fs: use fast api calls for existsSync - #49893

fs: use fast api calls for existsSync#49893littledivy wants to merge 8 commits intonodejs:mainnodejs/node:mainfrom littledivy:perf-existsSynclittledivy/node:perf-existsSyncCopy head branch name to clipboard

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!

joyeecheung Sep 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!

joyeecheung Sep 27, 2023

Choose a reason for hiding this comment

Uh oh!

targos Sep 28, 2023

Choose a reason for hiding this comment

Uh oh!

targos Sep 29, 2023

Choose a reason for hiding this comment

Uh oh!

joyeecheung Oct 2, 2023

Choose a reason for hiding this comment

Uh oh!

joyeecheung Oct 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-actions Bot 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

fs: use fast api calls for existsSync#49893
littledivy wants to merge 8 commits into
nodejs:mainfrom
littledivy:perf-existsSync

littledivy commented Sep 27, 2023 •
edited
Loading

joyeecheung Sep 27, 2023 •
edited
Loading