fs: improve error perf of sync *times

[CanadaHonk]

Results from i7 Windows laptop:

 confidence improvement accuracy (*) (**) (***) fs\bench-timesSync.js n=1000 func='futimes' type='existing' 1.30 % ±8.42% ±11.20% ±14.57% fs\bench-timesSync.js n=1000 func='futimes' type='non-existing' *** 105.93 % ±12.64% ±16.88% ±22.09% fs\bench-timesSync.js n=1000 func='lutimes' type='existing' 5.56 % ±8.48% ±11.29% ±14.73% fs\bench-timesSync.js n=1000 func='lutimes' type='non-existing' *** 62.51 % ±13.07% ±17.43% ±22.75% fs\bench-timesSync.js n=1000 func='utimes' type='existing' -1.04 % ±5.85% ±7.78% ±10.13% fs\bench-timesSync.js n=1000 func='utimes' type='non-existing' *** 61.88 % ±8.31% ±11.06% ±14.40% 

Ref: nodejs/performance#106

[nodejs-github-bot]

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

[CanadaHonk]

1 more review please? Thanks! @nodejs/fs @nodejs/cpp-reviewers

[anonrig]

@CanadaHonk can you resolve the conflicts?

[nodejs-github-bot]

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

[joyeecheung]

I think the diff can be a lot simpler if you just change the sync branch of the original implementations instead of repeating the code in a new binding..(and if you only introduce new bindings, the original sync branch would be dead code..) also I think this breaks --trace-sync-io?

[anonrig]

I think the diff can be a lot simpler if you just change the sync branch of the original implementations instead of repeating the code in a new binding..(and if you only introduce new bindings, the original sync branch would be dead code..) also I think this breaks --trace-sync-io?

I think differentiating promise and sync implementations are better to maintain and benchmark. I think this would make fast api implementations a lot harder.

[joyeecheung]

@anonrig

I think differentiating promise and sync implementations are better to maintain and benchmark.

The async one isn't related to promises. I've laid out my reasons about maintainability in #49593 (comment)

I think this would make fast api implementations a lot harder.

Can you elaborate on why? I don't think there is a lot of differences between the sync and the async version - note that we do not call the async callback immediately, this means we can include them in the fast API just fine. Essentially the sync v.s. async part really just comes down to what you pass to the libuv methods.

[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/54713/

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[anonrig]

Landed in 6bd77db

[Trott]

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

This was landed despite a failing Jenkins CI.

[anonrig]

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

This was landed despite a failing Jenkins CI.

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