Extract out fs validation functions #17682

maclover7 · 2017-12-14T14:27:26Z

Many different validations are repeated within fs.js, so I extracted them out to separate functions to try and DRY up the code and avoid duplicating them.

Checklist

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

Affected core subsystem(s)

fs

jasnell · 2017-12-14T16:05:52Z

lib/fs.js

If we're going to separate these out, would you mind using Error.captureStackTrace() to filter out the frame for the validation function.

e.g.

const err = new errors.TypeError('ERR_INVALID_ARG_TYPE', 'buffer', ['Buffer', 'Uint8Array']); Error.captureStackTrace(err, validateBuffer); throw err;

jasnell · 2017-12-14T16:06:35Z

I like this, but can we wait until #17667 lands then update to include those also?

apapirovski · 2017-12-14T16:14:00Z

Code LGTM but maybe benchmark it just to make sure nothing weirdly regresses.

lpinca · 2017-12-15T06:43:57Z

One thing I don't like is the removal of the last frame from the stack trace but I guess there is no other way if we want to keep the stack trace the same as before.

maclover7 · 2017-12-23T01:04:49Z

PTAL -- updated after @jasnell's PR landed

apapirovski · 2017-12-24T15:18:04Z

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

maclover7 · 2017-12-24T17:48:23Z

(no rush at all, but just waiting for a @jasnell approval before landing)

jasnell · 2017-12-27T14:37:23Z

still lgtm but I'd like to see a benchmark run.

maclover7 · 2017-12-31T22:37:35Z

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

maclover7 · 2017-12-31T22:38:20Z

@jasnell running benchmark CI at https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/81/ (first time running that job -- let me know if I configured it wrong :) )

joyeecheung · 2018-01-01T13:33:24Z

Benchmark results:

                                                                                  improvement confidence      p.value
 fs/bench-readdir.js n=10000                                                          -0.69 %            0.8221851122
 fs/bench-readdirSync.js n=10000                                                       1.19 %            0.1269799072
 fs/bench-realpath.js pathType="relative" n=10000                                     -0.39 %            0.8606992499
 fs/bench-realpath.js pathType="resolved" n=10000                                     -1.62 %            0.4597863345
 fs/bench-realpathSync.js pathType="relative" n=10000                                  0.81 %            0.3516556433
 fs/bench-realpathSync.js pathType="resolved" n=10000                                  0.85 %            0.6945707959
 fs/bench-stat.js statType="fstat" n=200000                                            0.20 %            0.9124787547
 fs/bench-stat.js statType="lstat" n=200000                                            1.39 %            0.5931550171
 fs/bench-stat.js statType="stat" n=200000                                             0.46 %            0.8648427618
 fs/bench-statSync.js statSyncType="fstatSync" n=1000000                              -0.55 %            0.3322678759
 fs/bench-statSync.js statSyncType="lstatSync" n=1000000                               0.31 %            0.7872484144
 fs/bench-statSync.js statSyncType="statSync" n=1000000                                0.21 %            0.4597629297
 fs/readfile.js concurrent=10 len=1024 dur=5                                          -1.69 %            0.1048860229
 fs/readfile.js concurrent=10 len=16777216 dur=5                                      -0.80 %            0.3697981467
 fs/readfile.js concurrent=1 len=1024 dur=5                                           -1.42 %            0.4533599114
 fs/readfile.js concurrent=1 len=16777216 dur=5                                        0.88 %            0.7638529174
 fs/readFileSync.js n=600000                                                          -3.78 %            0.3439485873
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType="asc"         0.82 %            0.6935378085
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType="buf"        -7.36 %        *** 0.0004981974
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType="utf"        -1.56 %            0.3167765711
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType="asc"     -2.18 %            0.2309335539
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType="buf"      1.38 %            0.6602041146
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType="utf"     11.20 %          * 0.0110945141
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType="asc"        -2.04 %            0.2632604078
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType="buf"        -1.52 %            0.5256513240
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType="utf"         0.72 %            0.7084833426
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType="asc"       -1.23 %            0.7168004653
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType="buf"        0.29 %            0.9028791881
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType="utf"        2.52 %            0.4328920710
 fs/write-stream-throughput.js size=1024 encodingType="asc" dur=5                     -3.36 %            0.3830714211
 fs/write-stream-throughput.js size=1024 encodingType="buf" dur=5                    -13.27 %          * 0.0197917940
 fs/write-stream-throughput.js size=1024 encodingType="utf" dur=5                      0.98 %            0.7265974114
 fs/write-stream-throughput.js size=1048576 encodingType="asc" dur=5                  -0.52 %            0.6714815125
 fs/write-stream-throughput.js size=1048576 encodingType="buf" dur=5                   3.19 %            0.7732731287
 fs/write-stream-throughput.js size=1048576 encodingType="utf" dur=5                  -4.52 %            0.0533034488
 fs/write-stream-throughput.js size=2 encodingType="asc" dur=5                       -18.21 %         ** 0.0036869252
 fs/write-stream-throughput.js size=2 encodingType="buf" dur=5                        -2.14 %            0.5764953715
 fs/write-stream-throughput.js size=2 encodingType="utf" dur=5                        -2.12 %            0.7745453484
 fs/write-stream-throughput.js size=65535 encodingType="asc" dur=5                    -3.69 %            0.4583537943
 fs/write-stream-throughput.js size=65535 encodingType="buf" dur=5                     0.79 %            0.9159899389
 fs/write-stream-throughput.js size=65535 encodingType="utf" dur=5                    -6.27 %            0.1084027753

There are a few performance hits in the stream throughput benchmarks that seem significant, although this PR does not seem to touch that part of code. I've started #83(waiting in queue right now) just to be sure @maclover7

maclover7 · 2018-01-02T23:10:16Z

@jasnell @joyeecheung It looks like the second round of benchmarks are much better than the first... is a third run needed? Is this safe to land?

joyeecheung · 2018-01-03T13:27:53Z

New benchmark results do not show any significant impact, I think it's safe to land.

                                                                                  improvement confidence    p.value
 fs/bench-readdir.js n=10000                                                           4.43 %            0.25035938
 fs/bench-readdirSync.js n=10000                                                       0.23 %            0.92332114
 fs/bench-realpath.js pathType="relative" n=10000                                      2.41 %            0.18837270
 fs/bench-realpath.js pathType="resolved" n=10000                                      2.79 %            0.13730962
 fs/bench-realpathSync.js pathType="relative" n=10000                                  0.14 %            0.87010835
 fs/bench-realpathSync.js pathType="resolved" n=10000                                  2.88 %            0.22536796
 fs/bench-stat.js statType="fstat" n=200000                                            2.71 %            0.15300635
 fs/bench-stat.js statType="lstat" n=200000                                            2.93 %            0.26604011
 fs/bench-stat.js statType="stat" n=200000                                            -1.03 %            0.67686451
 fs/bench-statSync.js statSyncType="fstatSync" n=1000000                               0.36 %            0.59151364
 fs/bench-statSync.js statSyncType="lstatSync" n=1000000                              -0.38 %            0.68432490
 fs/bench-statSync.js statSyncType="statSync" n=1000000                               -1.00 %            0.40065631
 fs/readfile.js concurrent=10 len=1024 dur=5                                           0.76 %            0.50907029
 fs/readfile.js concurrent=10 len=16777216 dur=5                                       0.13 %            0.88468325
 fs/readfile.js concurrent=1 len=1024 dur=5                                            1.00 %            0.47558070
 fs/readfile.js concurrent=1 len=16777216 dur=5                                        4.07 %            0.06571348
 fs/readFileSync.js n=600000                                                          -4.54 %            0.20217122
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType="asc"        -1.12 %            0.48733400
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType="buf"        -2.04 %            0.51379295
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType="utf"        -1.54 %            0.31347744
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType="asc"     -0.17 %            0.91459679
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType="buf"     -5.18 %            0.09443863
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType="utf"     -1.34 %            0.79498296
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType="asc"        -0.89 %            0.63531273
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType="buf"         1.21 %            0.67191413
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType="utf"        -0.38 %            0.78394829
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType="asc"       -6.24 %            0.14335794
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType="buf"       -0.26 %            0.92780750
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType="utf"        1.31 %            0.62066081
 fs/write-stream-throughput.js size=1024 encodingType="asc" dur=5                      2.14 %            0.50979394
 fs/write-stream-throughput.js size=1024 encodingType="buf" dur=5                      9.85 %            0.07099662
 fs/write-stream-throughput.js size=1024 encodingType="utf" dur=5                     -0.49 %            0.85268951
 fs/write-stream-throughput.js size=1048576 encodingType="asc" dur=5                   1.80 %            0.35226587
 fs/write-stream-throughput.js size=1048576 encodingType="buf" dur=5                   5.33 %            0.73214803
 fs/write-stream-throughput.js size=1048576 encodingType="utf" dur=5                  -5.49 %            0.09772179
 fs/write-stream-throughput.js size=2 encodingType="asc" dur=5                        -5.38 %            0.43068556
 fs/write-stream-throughput.js size=2 encodingType="buf" dur=5                         6.88 %            0.07779005
 fs/write-stream-throughput.js size=2 encodingType="utf" dur=5                         7.99 %            0.27306820
 fs/write-stream-throughput.js size=65535 encodingType="asc" dur=5                     5.43 %            0.30056025
 fs/write-stream-throughput.js size=65535 encodingType="buf" dur=5                    -6.95 %            0.33924458
 fs/write-stream-throughput.js size=65535 encodingType="utf" dur=5                    -0.68 %            0.86882016

apapirovski · 2018-01-03T18:54:38Z

New run of benchmarks: https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/84/

The runs setting should be at least 100 to get a good sample size, preferably higher. The results above have very low confidence level so it's impossible to tell whether these changes had any impact.

maclover7 · 2018-01-05T01:03:14Z

New benchmark results from https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/84/:

                                                                                  improvement confidence     p.value
 fs/bench-readdir.js n=10000                                                          -1.65 %            0.239633227
 fs/bench-readdirSync.js n=10000                                                      -0.08 %            0.836986871
 fs/bench-realpath.js pathType="relative" n=10000                                      0.50 %            0.477068802
 fs/bench-realpath.js pathType="resolved" n=10000                                     -0.13 %            0.848034244
 fs/bench-realpathSync.js pathType="relative" n=10000                                 -0.71 %          * 0.049475841
 fs/bench-realpathSync.js pathType="resolved" n=10000                                  0.15 %            0.764726643
 fs/bench-stat.js statType="fstat" n=200000                                           -0.80 %            0.308062144
 fs/bench-stat.js statType="lstat" n=200000                                            1.12 %            0.222583912
 fs/bench-stat.js statType="stat" n=200000                                            -0.81 %            0.409443518
 fs/bench-statSync.js statSyncType="fstatSync" n=1000000                              -0.30 %            0.673201199
 fs/bench-statSync.js statSyncType="lstatSync" n=1000000                              -0.98 %         ** 0.002277421
 fs/bench-statSync.js statSyncType="statSync" n=1000000                               -1.21 %         ** 0.002405144
 fs/readfile.js concurrent=10 len=1024 dur=5                                           0.42 %            0.211219097
 fs/readfile.js concurrent=10 len=16777216 dur=5                                       0.19 %            0.524627371
 fs/readfile.js concurrent=1 len=1024 dur=5                                           -1.13 %            0.070290435
 fs/readfile.js concurrent=1 len=16777216 dur=5                                        0.81 %            0.423644447
 fs/readFileSync.js n=600000                                                          -2.45 %            0.090920059
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType="asc"         0.49 %            0.573030366
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType="buf"        -1.00 %            0.381338206
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType="utf"        -0.32 %            0.593014633
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType="asc"     -0.89 %            0.314856551
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType="buf"     -2.17 %          * 0.040200429
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType="utf"     -2.86 %            0.168259032
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType="asc"        -1.98 %          * 0.015018724
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType="buf"        -0.07 %            0.938294852
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType="utf"        -0.11 %            0.862798269
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType="asc"       -0.77 %            0.591974006
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType="buf"       -1.17 %            0.231742254
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType="utf"       -1.80 %            0.130398138
 fs/write-stream-throughput.js size=1024 encodingType="asc" dur=5                      0.10 %            0.949376810
 fs/write-stream-throughput.js size=1024 encodingType="buf" dur=5                     -0.41 %            0.844558874
 fs/write-stream-throughput.js size=1024 encodingType="utf" dur=5                      1.30 %            0.180015631
 fs/write-stream-throughput.js size=1048576 encodingType="asc" dur=5                  -0.76 %            0.461714282
 fs/write-stream-throughput.js size=1048576 encodingType="buf" dur=5                  -6.90 %            0.119385216
 fs/write-stream-throughput.js size=1048576 encodingType="utf" dur=5                  -0.30 %            0.766930694
 fs/write-stream-throughput.js size=2 encodingType="asc" dur=5                        -1.09 %            0.670677189
 fs/write-stream-throughput.js size=2 encodingType="buf" dur=5                        -0.73 %            0.650901487
 fs/write-stream-throughput.js size=2 encodingType="utf" dur=5                        -5.30 %            0.054841256
 fs/write-stream-throughput.js size=65535 encodingType="asc" dur=5                     4.60 %          * 0.036493926
 fs/write-stream-throughput.js size=65535 encodingType="buf" dur=5                    -2.55 %            0.307207952
 fs/write-stream-throughput.js size=65535 encodingType="utf" dur=5                    -0.33 %            0.863831885

@apapirovski Does this look okay to land? Effects do not appear very negative.

joyeecheung · 2018-01-11T01:09:10Z

Landed in 6e3818f...8aec363, thanks!

PR-URL: #17682 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

maclover7 · 2018-01-13T01:37:41Z

Thank you for landing @joyeecheung!!

evanlucas · 2018-01-24T14:09:23Z

This isn't landing cleanly on v9.x. If it needs to be backported, can you please open a backport PR? Thanks!

MylesBorins · 2018-07-31T18:46:38Z

This touches a lot of code in fs and could make future backports difficult. With that being said it is a lot of churn.

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

maclover7 requested a review from jasnell December 14, 2017 14:27

jasnell reviewed Dec 14, 2017

View reviewed changes

jasnell added the fs Issues and PRs related to the fs subsystem / file system. label Dec 14, 2017

cjihrig approved these changes Dec 14, 2017

View reviewed changes

jasnell approved these changes Dec 14, 2017

View reviewed changes

lpinca approved these changes Dec 15, 2017

View reviewed changes

apapirovski approved these changes Dec 24, 2017

View reviewed changes

maclover7 mentioned this pull request Dec 24, 2017

fs: migrate fs_event_wrap to internal/errors #17851

Closed

3 tasks

maclover7 added 6 commits December 31, 2017 12:32

fs: extract out validateFd function

df6ceef

fs: extract out validateOffsetLengthRead function

3b7a7aa

fs: extract out validateBuffer function

0de5472

fs: extract out validateOffsetLengthWrite function

b2a7296

fs: extract out validatePath function

f225d4f

fs: extract out validateUint32 and validateLen functions

bfd1e14

joyeecheung approved these changes Jan 3, 2018

View reviewed changes

joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 3, 2018

joyeecheung mentioned this pull request Jan 10, 2018

fs: create consistent behavior for fs module across platforms #17927

Closed

4 tasks

joyeecheung closed this Jan 11, 2018

maclover7 deleted the jm-dry-fs branch January 13, 2018 01:37

TimothyGu removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 13, 2018

evanlucas added the backport-requested-v9.x label Jan 24, 2018

targos added dont-land-on-v9.x and removed backport-requested-v9.x labels Mar 30, 2018

MylesBorins added the backport-requested-v8.x label Jul 31, 2018

Uh oh!

Uh oh!

Extract out fs validation functions #17682

Extract out fs validation functions #17682

Uh oh!

Conversation

maclover7 commented Dec 14, 2017

Checklist

Affected core subsystem(s)

Uh oh!

jasnell Dec 14, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell commented Dec 14, 2017

Uh oh!

apapirovski commented Dec 14, 2017

Uh oh!

lpinca commented Dec 15, 2017

Uh oh!

maclover7 commented Dec 23, 2017

Uh oh!

apapirovski commented Dec 24, 2017

Uh oh!

maclover7 commented Dec 24, 2017

Uh oh!

jasnell commented Dec 27, 2017

Uh oh!

maclover7 commented Dec 31, 2017

Uh oh!

maclover7 commented Dec 31, 2017

Uh oh!

joyeecheung commented Jan 1, 2018

Uh oh!

maclover7 commented Jan 2, 2018

Uh oh!

joyeecheung commented Jan 3, 2018

Uh oh!

apapirovski commented Jan 3, 2018

Uh oh!

maclover7 commented Jan 5, 2018

Uh oh!

joyeecheung commented Jan 11, 2018

Uh oh!

maclover7 commented Jan 13, 2018

Uh oh!

evanlucas commented Jan 24, 2018

Uh oh!

MylesBorins commented Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

MylesBorins commented Jul 31, 2018 •

edited

Loading