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%

                                                                                              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