fs: refactor to use more primordials by aduh95 · Pull Request #36196 · nodejs/node · GitHub

Checklist

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

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

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

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%

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?

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

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

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?

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…

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.

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 (***)

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

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

/cc @mscdex

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

PR-URL: nodejs#36196 Reviewed-By: James M Snell <jasnell@gmail.com>

Landed in 8d6c2f2

PR-URL: #36196 Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 20, 2020

jasnell approved these changes Nov 20, 2020

View reviewed changes

aduh95 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 20, 2020

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 20, 2020

This comment has been minimized.

Sign in to view

aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 26, 2020

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 26, 2020

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 27, 2020

github-actions Bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 27, 2020

fs: refactor to use more primordials

8d6c2f2

PR-URL: nodejs#36196 Reviewed-By: James M Snell <jasnell@gmail.com>

aduh95 force-pushed the fs-primordials branch from 45200cc to 8d6c2f2 Compare November 27, 2020 17:41

aduh95 merged commit 8d6c2f2 into nodejs:master Nov 27, 2020

aduh95 deleted the fs-primordials branch November 27, 2020 17:41

danielleadams pushed a commit that referenced this pull request Dec 7, 2020

fs: refactor to use more primordials

c04a2df

PR-URL: #36196 Reviewed-By: James M Snell <jasnell@gmail.com>

danielleadams mentioned this pull request Dec 7, 2020

v15.4.0 proposal #36435

Merged

targos added the dont-land-on-v14.x label May 16, 2021

targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021

fs: refactor to use more primordials by aduh95 · Pull Request #36196 · nodejs/node · GitHub

Uh oh!

Uh oh!

fs: refactor to use more primordials - #36196

fs: refactor to use more primordials#36196aduh95 merged 1 commit intonodejs:masternodejs/node:masterfrom aduh95:fs-primordialsaduh95/node:fs-primordialsCopy head branch name to clipboard

Conversation

aduh95 commented Nov 20, 2020

Checklist

Uh oh!

This comment has been minimized.

nodejs-github-bot commented Nov 20, 2020

Uh oh!

mscdex commented Nov 20, 2020

Uh oh!

mscdex commented Nov 21, 2020

Uh oh!

aduh95 commented Nov 21, 2020

Uh oh!

mscdex commented Nov 21, 2020

Uh oh!

ExE-Boss commented Nov 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 commented Nov 21, 2020

Uh oh!

aduh95 commented Nov 22, 2020

Uh oh!

aduh95 commented Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 commented Nov 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Nov 26, 2020

Uh oh!

aduh95 commented Nov 27, 2020

Uh oh!

github-actions Bot commented Nov 27, 2020

Uh oh!

aduh95 commented Nov 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

fs: refactor to use more primordials#36196
aduh95 merged 1 commit into
nodejs:masterfrom
aduh95:fs-primordials

ExE-Boss commented Nov 21, 2020 •
edited
Loading

aduh95 commented Nov 25, 2020 •
edited
Loading

aduh95 commented Nov 26, 2020 •
edited
Loading