Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: improve error perf of sync chmod+fchmod #49859

Closed
wants to merge 2 commits into from

Conversation

CanadaHonk
Copy link
Member

@CanadaHonk CanadaHonk commented Sep 25, 2023

Results from i7 Windows laptop:

                                                  confidence improvement accuracy (*)    (**)   (***)
fs\bench-chmodSync.js n=1000 type='existing'                      2.46 %       ±9.77% ±13.01% ±16.95%
fs\bench-chmodSync.js n=1000 type='non-existing'         ***     74.14 %      ±14.58% ±19.42% ±25.32%
fs\bench-fchmodSync.js n=1000 type='existing'                     0.14 %       ±5.61%  ±7.48%  ±9.76%
fs\bench-fchmodSync.js n=1000 type='non-existing'        ***    118.16 %      ±21.27% ±28.52% ±37.58%

Ref: nodejs/performance#106

@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 the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Sep 25, 2023
@anonrig anonrig added the performance Issues and PRs related to the performance of Node.js. label Sep 25, 2023
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Sep 25, 2023

@nodejs/fs @nodejs/cpp-reviewers

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

anonrig commented Sep 27, 2023

@CanadaHonk can you resolve the conflicts?

@anonrig anonrig added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Sep 27, 2023
Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

LGTM but merge commits aren't supported by our tooling. Could you please rebase instead?

src/node_file.cc Outdated Show resolved Hide resolved
src/node_file.cc Outdated Show resolved Hide resolved
@CanadaHonk
Copy link
Member Author

LGTM but merge commits aren't supported by our tooling. Could you please rebase instead?

I think it's fine with commit-queue-squash?

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

This comment was marked as outdated.

@RaisinTen
Copy link
Contributor

The problem is that the Jenkins CI will fail because of the merge commit as it does right now

@CanadaHonk
Copy link
Member Author

Yep sorry, should be good now.

Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

No worries :)

@nodejs-github-bot
Copy link
Collaborator

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 27, 2023
@anonrig anonrig removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 27, 2023
@joyeecheung
Copy link
Member

My point is that we do not need to copy & repeat code from the original implementation for the improvement. The improvement comes from essentially very simple changes - instead of using the SyncCall that sets properties on the context, use something else to throw immediately, that should just be 2-3 lines of changes, instead of doing all the copying & repeating. We should keep the changes simple to avoid backport and git blame headaches.

Copy link
Member

@Qard Qard left a comment

Choose a reason for hiding this comment

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

I'm with Joyee in thinking the duplication is not great. Can we break out the common parts to another help, maybe? Or maybe we could make an alternate SyncCall(...) that does the direct throw rather than doing the ctx thing?

I'd really like to see us unifying how we're doing our libuv calls, especially to enable implementing native promise versions of things in the future rather than going through callbacks. We get a lot of benefits from using native promises rather than wrapping callback code, like async_hooks becomes unnecessary as PromiseHook already catches the barrier correctly, and we can get async stack traces out of it too.

@joyeecheung
Copy link
Member

joyeecheung commented Oct 2, 2023

#49913 has landed. Can you move the JS code back to lib/fs.js and switch to use SyncCallAndThrowOnError in the original implementation instead of introducing a new binding? Thanks.

@anonrig anonrig added commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Oct 11, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 11, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@anonrig
Copy link
Member

anonrig commented Oct 12, 2023

Landed in 6bd77db...d398529

anonrig pushed a commit that referenced this pull request Oct 12, 2023
PR-URL: #49859
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
anonrig added a commit that referenced this pull request Oct 12, 2023
PR-URL: #49859
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@anonrig anonrig closed this Oct 12, 2023
@Trott
Copy link
Member

Trott commented Oct 15, 2023

Landed in 6bd77db...d398529

It appears that this was manually landed despite a failing Jenkins CI. Was that intentional?

@anonrig
Copy link
Member

anonrig commented Oct 15, 2023

Landed in 6bd77db...d398529

It appears that this was manually landed despite a failing Jenkins CI. Was that intentional?

Yes, because the failing test was fixed in main branch. It was caused by test runner concurrency cli flag.

@tniessen
Copy link
Member

tniessen commented Oct 15, 2023

For what it's worth, I had a feeling that this PR and #49864 had been merged without passing CI, but I couldn't verify at the time because CI was locked down for security releases.

To be clear, manually merging PRs without a passing CI run is not acceptable for any collaborator. There may be rare exceptions with explicit TSC/releasers approval, but that is definitely not the case here. This is a clear violation of the collaborator guidelines, and silently doing so without even leaving an explaining comment only makes it worse.

@anonrig
Copy link
Member

anonrig commented Oct 15, 2023

Point taken @tniessen & @Trott. You're absolutely right.

alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
PR-URL: nodejs#49859
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
PR-URL: nodejs#49859
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this pull request Nov 11, 2023
PR-URL: #49859
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this pull request Nov 11, 2023
PR-URL: #49859
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants