Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: refactor stats array to be more generic #19714

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Mar 31, 2018

I have a WIP for BigInt integration of fs.Stats, which depends on the v8 6.7 update. This is part of the integration that does not depend on BigInt and does not change the API, but makes the related code more generic and reference less magic numbers and global variables.

  • Pass kFsStatsFieldsLength between JS and C++ instead of using the
    magic number 14
  • Pass the global stats array to the completion callback of
    asynchronous FSReqWrap similar to how the stats arrays are passed
    to the FSReqPromise resolvers
  • Abstract the stats converter and take an offset to compute the
    old stats in fs.watchFile
  • Use templates in node::FillStatsArray and FSReqPromise in preparation
    for BigInt intergration
  • Put the global stat array filler in node_internals.h because it is
    shared by node_file.cc and node_stat_watcher.cc
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 31, 2018
@joyeecheung
Copy link
Member Author

@mscdex
Copy link
Contributor

mscdex commented Mar 31, 2018

Did you run fs benchmarks after these changes?

@joyeecheung
Copy link
Member Author

joyeecheung commented Mar 31, 2018

Benchmark CI id is 149 (pending)

@mscdex
Copy link
Contributor

mscdex commented Mar 31, 2018

I mean even just locally, just the subset that would be affected by this.

@joyeecheung
Copy link
Member Author

joyeecheung commented Mar 31, 2018

@mscdex I only ran with --filter stat

                                                         confidence improvement accuracy (*)    (**)   (***)
 fs/bench-stat-promise.js statType='fstat' n=200000                     -1.96 %       ±4.23%  ±5.79%  ±7.90%
 fs/bench-stat-promise.js statType='lstat' n=200000                     -0.16 %       ±5.32%  ±7.29%  ±9.95%
 fs/bench-stat-promise.js statType='stat' n=200000                      -0.64 %       ±5.46%  ±7.51% ±10.30%
 fs/bench-stat.js statType='fstat' n=200000                              0.90 %       ±9.29% ±12.85% ±17.75%
 fs/bench-stat.js statType='lstat' n=200000                              0.13 %       ±9.95% ±13.72% ±18.86%
 fs/bench-stat.js statType='stat' n=200000                              -1.81 %      ±10.08% ±13.87% ±19.05%
 fs/bench-statSync.js statSyncType='fstatSync' n=1000000                 1.58 %       ±5.80%  ±7.98% ±10.93%
 fs/bench-statSync.js statSyncType='lstatSync' n=1000000                 0.68 %       ±5.24%  ±7.19%  ±9.82%
 fs/bench-statSync.js statSyncType='statSync' n=1000000                 -1.29 %       ±4.17%  ±5.75%  ±7.90%

@joyeecheung
Copy link
Member Author

Benchmark results, interestingly there are some significant improvements in APIs that used to reference the global statValues

https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/149

                                                                                  confidence improvement accuracy (*)    (**)   (***)
 fs/bench-readdir.js n=10000                                                              **     14.16 %      ±10.35% ±13.80% ±18.03%
 fs/bench-readdirSync.js n=10000                                                                 -0.59 %       ±7.72% ±10.27% ±13.36%
 fs/bench-realpath.js pathType='relative' n=10000                                                 1.34 %       ±4.77%  ±6.35%  ±8.26%
 fs/bench-realpath.js pathType='resolved' n=10000                                                -2.92 %       ±3.76%  ±5.01%  ±6.54%
 fs/bench-realpathSync.js pathType='relative' n=10000                                            -1.43 %       ±5.84%  ±7.79% ±10.18%
 fs/bench-realpathSync.js pathType='resolved' n=10000                                             1.80 %       ±4.63%  ±6.17%  ±8.05%
 fs/bench-stat.js statType='fstat' n=200000                                                       1.73 %       ±3.76%  ±5.01%  ±6.52%
 fs/bench-stat.js statType='lstat' n=200000                                                      -1.82 %       ±4.51%  ±6.01%  ±7.82%
 fs/bench-stat.js statType='stat' n=200000                                                       -0.05 %       ±5.51%  ±7.34%  ±9.56%
 fs/bench-stat-promise.js statType='fstat' n=200000                                              -0.31 %       ±3.24%  ±4.32%  ±5.64%
 fs/bench-stat-promise.js statType='lstat' n=200000                                        *     -3.51 %       ±3.14%  ±4.18%  ±5.45%
 fs/bench-stat-promise.js statType='stat' n=200000                                               -0.54 %       ±3.61%  ±4.80%  ±6.25%
 fs/bench-statSync.js statSyncType='fstatSync' n=1000000                                          0.63 %       ±4.52%  ±6.02%  ±7.84%
 fs/bench-statSync.js statSyncType='lstatSync' n=1000000                                         -0.79 %       ±1.73%  ±2.30%  ±3.00%
 fs/bench-statSync.js statSyncType='statSync' n=1000000                                    *     -2.08 %       ±1.77%  ±2.38%  ±3.14%
 fs/readfile.js concurrent=10 len=1024 dur=5                                                      1.08 %       ±1.74%  ±2.31%  ±3.01%
 fs/readfile.js concurrent=10 len=16777216 dur=5                                         ***     14.33 %       ±4.29%  ±5.71%  ±7.44%
 fs/readfile.js concurrent=1 len=1024 dur=5                                                      -0.60 %       ±3.94%  ±5.24%  ±6.82%
 fs/readfile.js concurrent=1 len=16777216 dur=5                                                  -0.23 %       ±5.58%  ±7.42%  ±9.68%
 fs/readfile-partitioned.js concurrent=10 len=1024 dur=5                                         -1.75 %       ±3.47%  ±4.61%  ±6.01%
 fs/readfile-partitioned.js concurrent=10 len=16777216 dur=5                             ***     11.81 %       ±5.55%  ±7.41%  ±9.69%
 fs/readfile-partitioned.js concurrent=1 len=1024 dur=5                                          -2.01 %       ±7.62% ±10.14% ±13.20%
 fs/readfile-partitioned.js concurrent=1 len=16777216 dur=5                                       3.77 %       ±5.41%  ±7.20%  ±9.38%
 fs/readFileSync.js n=600000                                                                     -4.59 %       ±9.73% ±12.95% ±16.87%
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType='asc'                    0.23 %       ±3.44%  ±4.57%  ±5.95%
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType='buf'                    1.70 %       ±3.28%  ±4.36%  ±5.68%
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType='utf'                    2.63 %       ±2.85%  ±3.80%  ±4.95%
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType='asc'                -1.42 %       ±3.62%  ±4.84%  ±6.34%
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType='buf'                -5.75 %       ±9.03% ±12.04% ±15.73%
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType='utf'                -2.85 %       ±9.04% ±12.03% ±15.67%
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType='asc'                    1.76 %       ±5.22%  ±6.98%  ±9.14%
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType='buf'                    3.31 %       ±4.29%  ±5.70%  ±7.42%
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType='utf'                    1.63 %       ±2.75%  ±3.67%  ±4.78%
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType='asc'                  -1.57 %       ±7.11%  ±9.46% ±12.31%
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType='buf'            *      8.25 %       ±7.19%  ±9.60% ±12.57%
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType='utf'                   0.89 %       ±9.33% ±12.41% ±16.16%
 fs/write-stream-throughput.js size=1024 encodingType='asc' dur=5                                 0.63 %       ±6.87%  ±9.15% ±11.94%
 fs/write-stream-throughput.js size=1024 encodingType='buf' dur=5                                 4.66 %      ±11.05% ±14.70% ±19.15%
 fs/write-stream-throughput.js size=1024 encodingType='utf' dur=5                                -5.44 %       ±5.99%  ±7.97% ±10.38%
 fs/write-stream-throughput.js size=1048576 encodingType='asc' dur=5                              1.16 %       ±2.16%  ±2.89%  ±3.81%
 fs/write-stream-throughput.js size=1048576 encodingType='buf' dur=5                             -5.16 %      ±28.00% ±37.26% ±48.50%
 fs/write-stream-throughput.js size=1048576 encodingType='utf' dur=5                              1.33 %       ±4.49%  ±6.00%  ±7.88%
 fs/write-stream-throughput.js size=2 encodingType='asc' dur=5                                    5.67 %      ±11.77% ±15.65% ±20.38%
 fs/write-stream-throughput.js size=2 encodingType='buf' dur=5                                   -1.05 %       ±3.79%  ±5.05%  ±6.57%
 fs/write-stream-throughput.js size=2 encodingType='utf' dur=5                                    2.24 %      ±11.13% ±14.81% ±19.30%
 fs/write-stream-throughput.js size=65535 encodingType='asc' dur=5                               -5.43 %       ±9.26% ±12.33% ±16.05%
 fs/write-stream-throughput.js size=65535 encodingType='buf' dur=5                                8.24 %      ±13.55% ±18.04% ±23.49%
 fs/write-stream-throughput.js size=65535 encodingType='utf' dur=5                               -9.02 %      ±10.15% ±13.50% ±17.57%

@joyeecheung
Copy link
Member Author

Rebased to address the conflicts with the newly-applied spaced-comment eslint rules.

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

- Pass kFsStatsFieldsLength between JS and C++ instead of using the
  magic number 14
- Pass the global stats array to the completion callback of
  asynchronous FSReqWrap similar to how the stats arrays are passed
  to the FSReqPromise resolvers
- Abstract the stats converter and take an offset to compute the
  old stats in fs.watchFile
- Use templates in node::FillStatsArray and FSReqPromise in preparation
  for BigInt intergration
- Put the global stat array filler in node_internals.h because it is
  shared by node_file.cc and node_stat_watcher.cc
@joyeecheung
Copy link
Member Author

@joyeecheung
Copy link
Member Author

CI is green, landing..

@joyeecheung
Copy link
Member Author

Landed in f7049a2, thanks!

@joyeecheung joyeecheung closed this Apr 4, 2018
joyeecheung added a commit that referenced this pull request Apr 4, 2018
- Pass kFsStatsFieldsLength between JS and C++ instead of using the
  magic number 14
- Pass the global stats array to the completion callback of
  asynchronous FSReqWrap similar to how the stats arrays are passed
  to the FSReqPromise resolvers
- Abstract the stats converter and take an offset to compute the
  old stats in fs.watchFile
- Use templates in node::FillStatsArray and FSReqPromise in preparation
  for BigInt intergration
- Put the global stat array filler in node_internals.h because it is
  shared by node_file.cc and node_stat_watcher.cc

PR-URL: #19714
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants