fs: improve error perf of sync chmod+fchmod

[CanadaHonk]

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]

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

[nodejs-github-bot]

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

[Trott]

@nodejs/fs @nodejs/cpp-reviewers

[anonrig]

@CanadaHonk can you resolve the conflicts?

[RaisinTen]

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

[CanadaHonk]

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

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

[RaisinTen]

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

[CanadaHonk]

Yep sorry, should be good now.

[RaisinTen]

No worries :)

[nodejs-github-bot]

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

[joyeecheung]

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.

[Qard]

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]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[anonrig]

Landed in 6bd77db...d398529

[Trott]

Landed in 6bd77db...d398529

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

[anonrig]

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]

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]

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