fs: refactor to use more primordials

[aduh95]

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[nodejs-github-bot]

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

[mscdex]

These changes seem to cause a consistent ~6% performance regression in at least the fstatSync() benchmark. I'm not sure if it's due to the ReflectApply() usage or something else....

[mscdex]

Yeah it looks like only fstatSync() and lstatSync() are affected:

fs/bench-statSync.js statSyncType='fstatSync' n=1000000 *** -7.85 % ±2.72% ±3.63% ±4.73% fs/bench-statSync.js statSyncType='lstatSync' n=1000000 *** -5.52 % ±2.47% ±3.30% ±4.31% 

[aduh95]

only fstatSync() and lstatSync() are affected

That might be coming from the change in the nullCheck function, called by getValidatedPath. That would mean StringPrototypeIncludes() is less efficient than <string>.includes(). Is there something we can do to workaround that?

[mscdex]

I don't know, but judging by at least these results and those from #36166 it seems that wholesale replacement of OOP constructs with these primordial functions can easily cause performance regressions, some more serious than others. I think we should be careful when making these kinds of changes.

Perhaps someone with better V8 knowledge (@nodejs/v8 ?) can chime in about the reason for these performance regressions and what we might be able to do about them....

[ExE-Boss]

I think it’s probably in part because using Reflect.apply requires creating an array, which is then converted into a list of arguments, whereas this step is skipped when calling a method directly.

Also, V8 before v8.0 didn’t support generating optimised code when using Reflect.apply or Function.prototype.{apply,bind,call} on built‑ins: https://v8.dev/blog/v8-release-80#optimizing-higher-order-builtins

[aduh95]

It loooks like v8 has moved away from using JS (https://github.com/v8/v8/blob/d6ead37d265d7215cf9c5f768f279e21bd170212/src/js/prologue.js#L152-L156) to C++ (https://github.com/v8/v8/blob/4b9b23521e6fd42373ebbcb20ebe03bf445494f9/src/bootstrapper.cc#L4619-L4620) to implement uncurryThis. Maybe that's something we should copy as well?

[aduh95]

Benchmark comparing master and this PR on top of #36221:

 confidence improvement accuracy (*) (**) (***) buffers-fill/bench-statSync.js statSyncType='fstatSync' n=1000000 *** -7.96 % ±1.31% ±1.75% ±2.30% buffers-fill/bench-statSync.js statSyncType='lstatSync' n=1000000 *** -5.56 % ±1.18% ±1.58% ±2.05% buffers-fill/bench-statSync.js statSyncType='statSync' n=1000000 *** -6.37 % ±1.02% ±1.36% ±1.78% 

We can see the regression is still there…

[aduh95]

I've rolled back to <string>.includes instead of the primordials, and used FunctionPrototypeCall instead of ReflectApply and got promising results on my local machine.

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/709/ (queued, will 404 until it starts)

EDIT: StringPrototypeIncludes seems to have little to no effect to the benchmark results.

[aduh95]

Benchmark results:

 confidence improvement accuracy (*) (**) (***) fs/bench-opendir.js bufferSize=1024 mode='async' dir='test/parallel' n=100 * 5.82 % ±5.77% ±7.71% ±10.10% fs/bench-opendir.js bufferSize=1024 mode='callback' dir='test/parallel' n=100 * -3.81 % ±3.25% ±4.33% ±5.64% fs/bench-statSync-failure.js statSyncType='throw' n=1000000 * 2.43 % ±2.32% ±3.09% ±4.02% fs/readfile.js concurrent=1 len=1024 duration=5 * 2.77 % ±2.52% ±3.36% ±4.38% 

Details

 confidence improvement accuracy (*) (**) (***) fs/bench-mkdirp.js n=10000 -1.05 % ±3.36% ±4.47% ±5.81% fs/bench-opendir.js bufferSize=1024 mode='async' dir='lib' n=100 -2.60 % ±6.65% ±8.85% ±11.52% fs/bench-opendir.js bufferSize=1024 mode='async' dir='test/parallel' n=100 * 5.82 % ±5.77% ±7.71% ±10.10% fs/bench-opendir.js bufferSize=1024 mode='callback' dir='lib' n=100 3.65 % ±6.82% ±9.09% ±11.87% fs/bench-opendir.js bufferSize=1024 mode='callback' dir='test/parallel' n=100 * -3.81 % ±3.25% ±4.33% ±5.64% fs/bench-opendir.js bufferSize=1024 mode='sync' dir='lib' n=100 -2.96 % ±7.85% ±10.44% ±13.59% fs/bench-opendir.js bufferSize=1024 mode='sync' dir='test/parallel' n=100 -3.82 % ±4.84% ±6.51% ±8.63% fs/bench-opendir.js bufferSize=32 mode='async' dir='lib' n=100 0.29 % ±6.84% ±9.11% ±11.86% fs/bench-opendir.js bufferSize=32 mode='async' dir='test/parallel' n=100 1.37 % ±5.07% ±6.74% ±8.78% fs/bench-opendir.js bufferSize=32 mode='callback' dir='lib' n=100 1.01 % ±7.48% ±9.95% ±12.95% fs/bench-opendir.js bufferSize=32 mode='callback' dir='test/parallel' n=100 2.55 % ±5.85% ±7.79% ±10.14% fs/bench-opendir.js bufferSize=32 mode='sync' dir='lib' n=100 -2.36 % ±6.69% ±8.90% ±11.60% fs/bench-opendir.js bufferSize=32 mode='sync' dir='test/parallel' n=100 -0.33 % ±1.97% ±2.64% ±3.46% fs/bench-opendir.js bufferSize=4 mode='async' dir='lib' n=100 -2.66 % ±6.64% ±8.84% ±11.51% fs/bench-opendir.js bufferSize=4 mode='async' dir='test/parallel' n=100 2.05 % ±3.29% ±4.38% ±5.70% fs/bench-opendir.js bufferSize=4 mode='callback' dir='lib' n=100 -2.76 % ±7.14% ±9.49% ±12.36% fs/bench-opendir.js bufferSize=4 mode='callback' dir='test/parallel' n=100 -0.56 % ±4.55% ±6.08% ±7.94% fs/bench-opendir.js bufferSize=4 mode='sync' dir='lib' n=100 0.54 % ±7.47% ±9.94% ±12.95% fs/bench-opendir.js bufferSize=4 mode='sync' dir='test/parallel' n=100 0.53 % ±1.51% ±2.01% ±2.61% fs/bench-readdir.js withFileTypes='false' dir='lib' n=10 1.06 % ±3.83% ±5.09% ±6.63% fs/bench-readdir.js withFileTypes='false' dir='test/parallel' n=10 -1.43 % ±2.62% ±3.49% ±4.56% fs/bench-readdir.js withFileTypes='true' dir='lib' n=10 0.09 % ±3.00% ±3.99% ±5.19% fs/bench-readdir.js withFileTypes='true' dir='test/parallel' n=10 0.19 % ±3.59% ±4.78% ±6.24% fs/bench-readdirSync.js withFileTypes='false' dir='lib' n=10 4.64 % ±5.47% ±7.30% ±9.56% fs/bench-readdirSync.js withFileTypes='false' dir='test/parallel' n=10 -1.37 % ±5.22% ±6.94% ±9.03% fs/bench-readdirSync.js withFileTypes='true' dir='lib' n=10 2.18 % ±6.18% ±8.27% ±10.86% fs/bench-readdirSync.js withFileTypes='true' dir='test/parallel' n=10 2.94 % ±3.42% ±4.55% ±5.93% fs/bench-realpath.js pathType='relative' n=10000 -1.69 % ±3.03% ±4.04% ±5.26% fs/bench-realpath.js pathType='resolved' n=10000 -1.00 % ±3.97% ±5.28% ±6.88% fs/bench-realpathSync.js pathType='relative' n=10000 -0.64 % ±4.02% ±5.35% ±6.96% fs/bench-realpathSync.js pathType='resolved' n=10000 -0.24 % ±3.52% ±4.69% ±6.10% fs/bench-stat.js statType='fstat' n=200000 -2.80 % ±3.79% ±5.05% ±6.57% fs/bench-stat.js statType='lstat' n=200000 -1.32 % ±4.31% ±5.73% ±7.46% fs/bench-stat.js statType='stat' n=200000 -1.09 % ±4.54% ±6.05% ±7.88% fs/bench-stat-promise.js statType='fstat' n=200000 1.54 % ±3.09% ±4.11% ±5.35% fs/bench-stat-promise.js statType='lstat' n=200000 -0.12 % ±3.33% ±4.43% ±5.76% fs/bench-stat-promise.js statType='stat' n=200000 1.80 % ±3.69% ±4.92% ±6.41% fs/bench-statSync-failure.js statSyncType='noThrow' n=1000000 -1.92 % ±3.25% ±4.34% ±5.67% fs/bench-statSync-failure.js statSyncType='throw' n=1000000 * 2.43 % ±2.32% ±3.09% ±4.02% fs/bench-statSync.js statSyncType='fstatSync' n=1000000 -0.05 % ±1.08% ±1.44% ±1.88% fs/bench-statSync.js statSyncType='lstatSync' n=1000000 -1.27 % ±1.98% ±2.64% ±3.43% fs/bench-statSync.js statSyncType='statSync' n=1000000 -1.60 % ±1.96% ±2.64% ±3.48% fs/readfile.js concurrent=10 len=1024 duration=5 1.69 % ±7.80% ±10.39% ±13.55% fs/readfile.js concurrent=10 len=16777216 duration=5 1.33 % ±4.30% ±5.72% ±7.44% fs/readfile.js concurrent=1 len=1024 duration=5 * 2.77 % ±2.52% ±3.36% ±4.38% fs/readfile.js concurrent=1 len=16777216 duration=5 -3.39 % ±12.40% ±16.50% ±21.48% fs/readfile-partitioned.js concurrent=10 len=1024 dur=5 0.34 % ±9.08% ±12.08% ±15.73% fs/readfile-partitioned.js concurrent=10 len=16777216 dur=5 -1.03 % ±6.22% ±8.28% ±10.78% fs/readfile-partitioned.js concurrent=1 len=1024 dur=5 5.75 % ±8.70% ±11.58% ±15.08% fs/readfile-partitioned.js concurrent=1 len=16777216 dur=5 1.63 % ±4.47% ±5.96% ±7.76% fs/readFileSync.js n=600000 -2.39 % ±5.23% ±6.96% ±9.06% fs/read-stream-throughput.js n=1024 highWaterMark=1024 filesize=1024000 encodingType='asc' 0.06 % ±2.80% ±3.72% ±4.85% fs/read-stream-throughput.js n=1024 highWaterMark=1024 filesize=1024000 encodingType='buf' -0.16 % ±2.47% ±3.29% ±4.28% fs/read-stream-throughput.js n=1024 highWaterMark=1024 filesize=1024000 encodingType='utf' -1.57 % ±2.00% ±2.65% ±3.46% fs/read-stream-throughput.js n=1024 highWaterMark=1048576 filesize=1024000 encodingType='asc' -1.85 % ±8.20% ±10.91% ±14.21% fs/read-stream-throughput.js n=1024 highWaterMark=1048576 filesize=1024000 encodingType='buf' 10.25 % ±22.24% ±29.60% ±38.55% fs/read-stream-throughput.js n=1024 highWaterMark=1048576 filesize=1024000 encodingType='utf' 6.23 % ±18.38% ±24.46% ±31.83% fs/read-stream-throughput.js n=1024 highWaterMark=4096 filesize=1024000 encodingType='asc' -1.90 % ±2.52% ±3.36% ±4.37% fs/read-stream-throughput.js n=1024 highWaterMark=4096 filesize=1024000 encodingType='buf' -2.83 % ±3.92% ±5.22% ±6.80% fs/read-stream-throughput.js n=1024 highWaterMark=4096 filesize=1024000 encodingType='utf' -0.21 % ±5.23% ±6.96% ±9.06% fs/read-stream-throughput.js n=1024 highWaterMark=65535 filesize=1024000 encodingType='asc' -5.92 % ±6.94% ±9.24% ±12.05% fs/read-stream-throughput.js n=1024 highWaterMark=65535 filesize=1024000 encodingType='buf' 0.36 % ±13.29% ±17.71% ±23.11% fs/read-stream-throughput.js n=1024 highWaterMark=65535 filesize=1024000 encodingType='utf' 3.95 % ±5.37% ±7.15% ±9.30% fs/write-stream-throughput.js size=1024 encodingType='asc' dur=5 1.61 % ±5.24% ±6.97% ±9.07% fs/write-stream-throughput.js size=1024 encodingType='buf' dur=5 -5.33 % ±9.48% ±12.62% ±16.42% fs/write-stream-throughput.js size=1024 encodingType='utf' dur=5 -1.90 % ±5.83% ±7.75% ±10.09% fs/write-stream-throughput.js size=1048576 encodingType='asc' dur=5 -1.51 % ±7.72% ±10.27% ±13.37% fs/write-stream-throughput.js size=1048576 encodingType='buf' dur=5 3.20 % ±10.82% ±14.40% ±18.74% fs/write-stream-throughput.js size=1048576 encodingType='utf' dur=5 -1.30 % ±3.44% ±4.57% ±5.95% fs/write-stream-throughput.js size=2 encodingType='asc' dur=5 3.04 % ±13.02% ±17.32% ±22.55% fs/write-stream-throughput.js size=2 encodingType='buf' dur=5 4.76 % ±9.90% ±13.17% ±17.15% fs/write-stream-throughput.js size=2 encodingType='utf' dur=5 7.51 % ±12.53% ±16.67% ±21.71% fs/write-stream-throughput.js size=65535 encodingType='asc' dur=5 -0.06 % ±10.79% ±14.35% ±18.68% fs/write-stream-throughput.js size=65535 encodingType='buf' dur=5 2.46 % ±11.36% ±15.12% ±19.68% fs/write-stream-throughput.js size=65535 encodingType='utf' dur=5 -4.25 % ±9.06% ±12.07% ±15.73% Be aware that when doing many comparisons the risk of a false-positive result increases. In this case there are 75 comparisons, you can thus expect the following amount of false-positive results: 3.75 false positives, when considering a 5% risk acceptance (*, **, ***), 0.75 false positives, when considering a 1% risk acceptance (**, ***), 0.07 false positives, when considering a 0.1% risk acceptance (***) 

[nodejs-github-bot]

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

[aduh95]

If there is no objections, I'm planning on landing this later today.

/cc @mscdex

[github-actions]

Commit Queue failed

- Loading data for nodejs/node/pull/36196 ✔ Done loading data for nodejs/node/pull/36196 ----------------------------------- PR info ------------------------------------ Title fs: refactor to use more primordials (#36196) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch aduh95:fs-primordials -> nodejs:master Labels author ready, lib / src Commits 5 - fs: refactor to use more primordials - fixup! fs: refactor to use more primordials - fixup! fs: refactor to use more primordials - fixup! fs: refactor to use more primordials - fixup! fs: refactor to use more primordials Committers 1 - Antoine du Hamel PR-URL: https://github.com/nodejs/node/pull/36196 Reviewed-By: James M Snell ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/36196 Reviewed-By: James M Snell -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last review: ⚠ - fixup! fs: refactor to use more primordials ⚠ - fixup! fs: refactor to use more primordials ⚠ - fixup! fs: refactor to use more primordials ⚠ - fixup! fs: refactor to use more primordials ✔ Last GitHub Actions successful ℹ Last Full PR CI on 2020-11-26T00:43:41Z: https://ci.nodejs.org/job/node-test-pull-request/34566/ ℹ Last Benchmark CI on 2020-11-25T14:43:15Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/709/ - Querying data for job/node-test-pull-request/34566/ ✔ Build data downloaded ✔ Last Jenkins CI successful ℹ This PR was created on Fri, 20 Nov 2020 14:36:57 GMT ✔ Approvals: 1 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/36196#pullrequestreview-535540379 -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/387516680

[aduh95]

Landed in 8d6c2f2