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 (!isUint8Array(buffer))
+    throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'buffer',
+                               ['Buffer', 'Uint8Array']);
+}


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

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

maclover7 force-pushed the jm-dry-fs branch 3 times, most recently from 2b1400e to e8bc54b Compare December 23, 2017 01:02

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

maclover7 force-pushed the jm-dry-fs branch from e8bc54b to bfd1e14 Compare December 31, 2017 17:48

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract out fs validation functions #17682

Extract out fs validation functions #17682

maclover7 commented Dec 14, 2017

jasnell Dec 14, 2017

jasnell commented Dec 14, 2017

apapirovski commented Dec 14, 2017

lpinca commented Dec 15, 2017

maclover7 commented Dec 23, 2017

apapirovski commented Dec 24, 2017

maclover7 commented Dec 24, 2017

jasnell commented Dec 27, 2017

maclover7 commented Dec 31, 2017

maclover7 commented Dec 31, 2017

joyeecheung commented Jan 1, 2018

maclover7 commented Jan 2, 2018

joyeecheung commented Jan 3, 2018

joyeecheung commented Jan 11, 2018

maclover7 commented Jan 13, 2018

evanlucas commented Jan 24, 2018

MylesBorins commented Jul 31, 2018 •

edited

Loading

Extract out fs validation functions #17682

Extract out fs validation functions #17682

Conversation

maclover7 commented Dec 14, 2017

Checklist

Affected core subsystem(s)

jasnell Dec 14, 2017

Choose a reason for hiding this comment

jasnell commented Dec 14, 2017

apapirovski commented Dec 14, 2017

lpinca commented Dec 15, 2017

maclover7 commented Dec 23, 2017

apapirovski commented Dec 24, 2017

maclover7 commented Dec 24, 2017

jasnell commented Dec 27, 2017

maclover7 commented Dec 31, 2017

maclover7 commented Dec 31, 2017

joyeecheung commented Jan 1, 2018

maclover7 commented Jan 2, 2018

joyeecheung commented Jan 3, 2018

joyeecheung commented Jan 11, 2018

maclover7 commented Jan 13, 2018

evanlucas commented Jan 24, 2018

MylesBorins commented Jul 31, 2018 • edited Loading

MylesBorins commented Jul 31, 2018 •

edited

Loading