GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fs: use fast api calls for `existsSync` by littledivy · Pull Request #49893 · nodejs/node

Conversation

@littledivy

@littledivy littledivy commented Sep 27, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
panva, daniellockyer, billywhizz, and H4ad reacted with heart emoji bricss, debadree25, daniellockyer, CanadaHonk, H4ad, and styfle reacted with rocket emoji
All reactions
  • ❤️ 4 reactions
  • 🚀 6 reactions
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to file-system APIs and the fs module. needs-ci PRs that need a full CI run. labels Sep 27, 2023
littledivy added 2 commits September 27, 2023 17:46
@littledivy
fs: improve performance of existsSync
f413971
@littledivy
Format
dd08af9
@littledivy
littledivy force-pushed the perf-existsSync branch from e5014d5 to dd08af9 Compare September 27, 2023 12:17
@panva 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. Only starts once the PR has an approving review. labels Sep 27, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 27, 2023
@nodejs-github-bot

nodejs-github-bot commented Sep 27, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
anonrig
anonrig approved these changes Sep 27, 2023
View reviewed changes
@anonrig anonrig added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Sep 27, 2023
@anonrig

anonrig commented Sep 27, 2023

Copy link
Copy Markdown
Member

Can you resolve the conflict @littledivy?
littledivy reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
debadree25
debadree25 reviewed Sep 27, 2023
View reviewed changes

@debadree25 debadree25 left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
anonrig reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@anonrig anonrig added the needs-benchmark-ci PRs that need a benchmark CI run. label Sep 27, 2023
mscdex
mscdex reviewed Sep 27, 2023
View reviewed changes
Comment thread src/node_file.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
@anonrig anonrig removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Sep 27, 2023
@littledivy
Merge branch 'main' into perf-existsSync
ac4be44
@anonrig anonrig added the commit-queue-squash PRs the Commit Queue should land as one squashed commit. label Sep 27, 2023
littledivy added 2 commits September 27, 2023 19:33
@littledivy
Add test for fast call
69736c2
@littledivy
Fix lint js
176cbdc
anonrig
anonrig reviewed Sep 27, 2023
View reviewed changes
Comment thread test/parallel/test-fs-exists.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@littledivy
Add test for true existsSync
8d331ed
CanadaHonk
CanadaHonk reviewed Sep 27, 2023
View reviewed changes
Comment thread src/node_file.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
tniessen
tniessen reviewed Sep 27, 2023
View reviewed changes
Comment thread src/node_file.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
littledivy added 2 commits September 27, 2023 21:14
@littledivy
Use SetFastMethod
7a37e48
@littledivy
Format
e2e7341
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 27, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 27, 2023
@nodejs-github-bot

nodejs-github-bot commented Sep 27, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 27, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 27, 2023
@nodejs-github-bot

nodejs-github-bot commented Sep 27, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tniessen
tniessen approved these changes Sep 27, 2023
View reviewed changes
joyeecheung
joyeecheung reviewed Sep 27, 2023
View reviewed changes
Comment thread src/node_file.cc
Comment on lines +1120 to +1135
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

@joyeecheung joyeecheung Sep 27, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tniessen and SukkaW reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
joyeecheung
joyeecheung reviewed Sep 27, 2023
View reviewed changes
Comment thread 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));

@joyeecheung joyeecheung Sep 27, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@targos targos Sep 28, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
debadree25 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@targos targos Sep 29, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@joyeecheung joyeecheung Oct 2, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@joyeecheung joyeecheung Oct 2, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@github-actions github-actions Bot mentioned this pull request Sep 28, 2023
CI Reliability 2023-09-28 nodejs/reliability#677
Open
19 tasks
anonrig
anonrig reviewed Sep 29, 2023
View reviewed changes
Comment thread src/node_file.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
@anonrig anonrig mentioned this pull request Oct 3, 2023
fix: use fs.existsSync to avoid race condition vercel/next.js#56387
Merged
huozhi pushed a commit to vercel/next.js that referenced this pull request Oct 3, 2023
@anonrig @kodiakhq
fix: use fs.existsSync to avoid race condition (#56387)
a46c5af 
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>
fhinkel
fhinkel approved these changes Jan 28, 2024
View reviewed changes
@aduh95

aduh95 commented May 11, 2024

Copy link
Copy Markdown
Contributor

This needs a rebase.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@avivkeller

avivkeller commented Aug 9, 2024

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@avivkeller avivkeller added the stalled Issues and PRs manually marked as stalled and scheduled for automatic closure. label Aug 9, 2024
@github-actions

github-actions Bot commented Aug 9, 2024

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@avivkeller avivkeller closed this Oct 15, 2024
@littledivy

littledivy commented Oct 15, 2024

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@avivkeller

avivkeller commented Oct 15, 2024

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonrig anonrig anonrig approved these changes
@tniessen tniessen tniessen approved these changes
@joyeecheung joyeecheung joyeecheung left review comments
+5 more reviewers
@mscdex mscdex mscdex left review comments
@targos targos targos left review comments
@CanadaHonk CanadaHonk CanadaHonk left review comments
@debadree25 debadree25 debadree25 left review comments
@fhinkel fhinkel fhinkel approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash PRs the Commit Queue should land as one squashed commit. fs Issues and PRs related to file-system APIs and the fs module. needs-benchmark-ci PRs that need a benchmark CI run. needs-ci PRs that need a full CI run. performance Issues and PRs related to the performance of Node.js. stalled Issues and PRs manually marked as stalled and scheduled for automatic closure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

13 participants

@littledivy @nodejs-github-bot @anonrig @aduh95 @avivkeller @mscdex @fhinkel @targos @tniessen @joyeecheung @CanadaHonk @debadree25 @panva
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.