fs: use fast api calls for `existsSync` #49893

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 ?

src/node_file.cc

test/parallel/test-fs-exists.js

src/node_file.cc

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

src/node_file.cc

+  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

test/parallel/test-fs-exists.js

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

src/node_file.cc

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.

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

src/node_file.cc Show resolved Hide resolved

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

test/parallel/test-fs-exists.js Show resolved Hide resolved

Add test for true existsSync

8d331ed

CanadaHonk reviewed Sep 27, 2023

View reviewed changes

src/node_file.cc Outdated Show resolved Hide resolved

tniessen reviewed Sep 27, 2023

View reviewed changes

src/node_file.cc Outdated Show resolved Hide resolved

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

src/node_file.cc Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: use fast api calls for `existsSync` #49893

fs: use fast api calls for `existsSync` #49893

littledivy commented Sep 27, 2023 •

edited

nodejs-github-bot commented Sep 27, 2023

anonrig commented Sep 27, 2023

debadree25 left a comment

nodejs-github-bot commented Sep 27, 2023

nodejs-github-bot commented Sep 27, 2023

joyeecheung Sep 27, 2023 •

edited

joyeecheung Sep 27, 2023

targos Sep 28, 2023

targos Sep 29, 2023

joyeecheung Oct 2, 2023

joyeecheung Oct 2, 2023

aduh95 commented May 11, 2024

fs: use fast api calls for existsSync #49893

Are you sure you want to change the base?

fs: use fast api calls for existsSync #49893

Conversation

littledivy commented Sep 27, 2023 • edited

nodejs-github-bot commented Sep 27, 2023

anonrig commented Sep 27, 2023

debadree25 left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Sep 27, 2023

nodejs-github-bot commented Sep 27, 2023

joyeecheung Sep 27, 2023 • edited

Choose a reason for hiding this comment

joyeecheung Sep 27, 2023

Choose a reason for hiding this comment

targos Sep 28, 2023

Choose a reason for hiding this comment

targos Sep 29, 2023

Choose a reason for hiding this comment

joyeecheung Oct 2, 2023

Choose a reason for hiding this comment

joyeecheung Oct 2, 2023

Choose a reason for hiding this comment

aduh95 commented May 11, 2024

fs: use fast api calls for `existsSync` #49893

fs: use fast api calls for `existsSync` #49893

littledivy commented Sep 27, 2023 •

edited

joyeecheung Sep 27, 2023 •

edited