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.

Sign up for GitHub

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

Jump to bottom

fs: replace duplicate conditions by a function #18717

Closed
wants to merge 1 commit into from
Closed

fs: replace duplicate conditions by a function #18717

wants to merge 1 commit into from

Conversation

daynin
Copy link
Contributor

@daynin daynin commented Feb 11, 2018

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
Affected core subsystem(s)

fs

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Feb 11, 2018
cjihrig
cjihrig reviewed Feb 11, 2018
View reviewed changes
lib/fs.js Outdated
@@ -346,6 +346,12 @@ Stats.prototype.isSocket = function() {

const statValues = binding.statValues;

function isPathPart(fileType) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isPathPart() might not be the best name. Maybe isFileType()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cjihrig yeah, It sounds much better! Thank you!

starkwang
starkwang approved these changes Feb 12, 2018
View reviewed changes
Copy link
Contributor

@starkwang starkwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

jasnell
jasnell approved these changes Feb 14, 2018
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but we should run the benchmark. It shouldn't have any significant changes but fs is particularly sensitive.

@daynin
Copy link
Contributor Author

daynin commented Feb 14, 2018 •
edited
Loading

@jasnell Hi!

Looks like there are no significant changes

                                                                                  confidence improvement accuracy (*)   (**)  (***)
 fs/bench-readdir.js n=10000                                                             ***      0.89 %       ±0.51% ±0.68% ±0.88%
 fs/bench-readdirSync.js n=10000                                                           *     -0.56 %       ±0.44% ±0.58% ±0.76%
 fs/bench-realpath.js pathType="relative" n=10000                                                 0.62 %       ±0.66% ±0.88% ±1.15%
 fs/bench-realpath.js pathType="resolved" n=10000                                         **      1.24 %       ±0.92% ±1.22% ±1.59%
 fs/bench-realpathSync.js pathType="relative" n=10000                                     **      1.75 %       ±1.11% ±1.48% ±1.93%
 fs/bench-realpathSync.js pathType="resolved" n=10000                                    ***      3.08 %       ±1.34% ±1.79% ±2.34%
 fs/bench-stat-promise.js statType="fstat" n=200000                                       **     -1.34 %       ±0.84% ±1.12% ±1.45%
 fs/bench-stat-promise.js statType="lstat" n=200000                                              -0.51 %       ±1.46% ±1.94% ±2.53%
 fs/bench-stat-promise.js statType="stat" n=200000                                               -1.46 %       ±2.00% ±2.66% ±3.46%
 fs/bench-stat.js statType="fstat" n=200000                                                       0.39 %       ±0.58% ±0.77% ±1.01%
 fs/bench-stat.js statType="lstat" n=200000                                                      -0.03 %       ±0.61% ±0.81% ±1.06%
 fs/bench-stat.js statType="stat" n=200000                                                        0.21 %       ±0.56% ±0.75% ±0.98%
 fs/bench-statSync.js statSyncType="fstatSync" n=1000000                                 ***      2.15 %       ±0.50% ±0.67% ±0.88%
 fs/bench-statSync.js statSyncType="lstatSync" n=1000000                                          1.24 %       ±2.22% ±2.95% ±3.84%
 fs/bench-statSync.js statSyncType="statSync" n=1000000                                   **     -3.63 %       ±2.47% ±3.29% ±4.29%
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType="asc"                    0.66 %       ±0.92% ±1.22% ±1.59%
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType="buf"             *      1.17 %       ±0.93% ±1.24% ±1.62%
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType="utf"            **      1.73 %       ±1.10% ±1.47% ±1.92%
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType="asc"                 0.16 %       ±0.57% ±0.76% ±0.99%
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType="buf"         **      0.72 %       ±0.51% ±0.68% ±0.89%
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType="utf"        ***      6.46 %       ±0.40% ±0.53% ±0.69%
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType="asc"                    0.05 %       ±1.16% ±1.54% ±2.00%
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType="buf"             *      0.89 %       ±0.87% ±1.16% ±1.51%
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType="utf"           ***      4.26 %       ±0.63% ±0.85% ±1.10%
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType="asc"                   0.05 %       ±0.60% ±0.80% ±1.04%
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType="buf"          ***      1.03 %       ±0.38% ±0.51% ±0.66%
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType="utf"          ***      6.58 %       ±0.41% ±0.55% ±0.71%
 fs/readfile-partitioned.js concurrent=1 len=1024 dur=5                                           0.45 %       ±0.60% ±0.80% ±1.04%
 fs/readfile-partitioned.js concurrent=1 len=16777216 dur=5                                       0.39 %       ±0.83% ±1.11% ±1.45%
 fs/readfile-partitioned.js concurrent=10 len=1024 dur=5                                         -0.40 %       ±0.42% ±0.55% ±0.72%
 fs/readfile-partitioned.js concurrent=10 len=16777216 dur=5                                      0.24 %       ±0.44% ±0.59% ±0.76%
 fs/readfile.js concurrent=1 len=1024 dur=5                                              ***      0.91 %       ±0.44% ±0.59% ±0.77%
 fs/readfile.js concurrent=1 len=16777216 dur=5                                           **      0.45 %       ±0.32% ±0.43% ±0.56%
 fs/readfile.js concurrent=10 len=1024 dur=5                                                     -0.03 %       ±0.61% ±0.82% ±1.07%
 fs/readfile.js concurrent=10 len=16777216 dur=5                                                  0.56 %       ±0.70% ±0.93% ±1.22%
 fs/readFileSync.js n=600000                                                                      0.14 %       ±1.93% ±2.58% ±3.37%
 fs/write-stream-throughput.js size=1024 encodingType="asc" dur=5                                -0.95 %       ±2.19% ±2.91% ±3.80%
 fs/write-stream-throughput.js size=1024 encodingType="buf" dur=5                                -0.18 %       ±2.21% ±2.94% ±3.82%
 fs/write-stream-throughput.js size=1024 encodingType="utf" dur=5                                -0.80 %       ±1.84% ±2.45% ±3.20%
 fs/write-stream-throughput.js size=1048576 encodingType="asc" dur=5                              0.13 %       ±1.87% ±2.49% ±3.25%
 fs/write-stream-throughput.js size=1048576 encodingType="buf" dur=5                             -0.83 %       ±1.86% ±2.47% ±3.22%
 fs/write-stream-throughput.js size=1048576 encodingType="utf" dur=5                              0.06 %       ±1.99% ±2.65% ±3.45%
 fs/write-stream-throughput.js size=2 encodingType="asc" dur=5                                    1.41 %       ±2.49% ±3.31% ±4.31%
 fs/write-stream-throughput.js size=2 encodingType="buf" dur=5                             *      0.91 %       ±0.74% ±0.98% ±1.28%
 fs/write-stream-throughput.js size=2 encodingType="utf" dur=5                                   -1.63 %       ±2.57% ±3.42% ±4.46%
 fs/write-stream-throughput.js size=65535 encodingType="asc" dur=5                               -0.75 %       ±1.85% ±2.47% ±3.23%
 fs/write-stream-throughput.js size=65535 encodingType="buf" dur=5                               -0.67 %       ±2.07% ±2.75% ±3.59%
 fs/write-stream-throughput.js size=65535 encodingType="utf" dur=5                               -0.15 %       ±1.78% ±2.37% ±3.10%

P.S.
I'm not sure about these improvements:

 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType="utf"          ***      6.58 %       ±0.41% ±0.55% ±0.71%
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType="utf"           ***      4.26 %       ±0.63% ±0.85% ±1.10%
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType="utf"        ***      6.46 %       ±0.40% ±0.53% ±0.69%

I'll try to repeat it

@daynin
Copy link
Contributor Author

daynin commented Feb 15, 2018

100 runs:

                                                                                  confidence improvement accuracy (*)   (**)  (***)
 fs/bench-readdir.js n=10000                                                                     -0.25 %       ±0.31% ±0.41% ±0.52%
 fs/bench-readdirSync.js n=10000                                                                 -0.09 %       ±0.27% ±0.36% ±0.46%
 fs/bench-realpath.js pathType='relative' n=10000                                                -0.10 %       ±0.40% ±0.53% ±0.68%
 fs/bench-realpath.js pathType='resolved' n=10000                                                -0.20 %       ±0.49% ±0.64% ±0.83%
 fs/bench-realpathSync.js pathType='relative' n=10000                                            -0.11 %       ±0.51% ±0.68% ±0.87%
 fs/bench-realpathSync.js pathType='resolved' n=10000                                            -0.44 %       ±0.70% ±0.92% ±1.18%
 fs/bench-stat-promise.js statType='fstat' n=200000                                      ***     -0.93 %       ±0.50% ±0.66% ±0.84%
 fs/bench-stat-promise.js statType='lstat' n=200000                                               0.62 %       ±1.02% ±1.34% ±1.72%
 fs/bench-stat-promise.js statType='stat' n=200000                                         *      1.02 %       ±1.00% ±1.31% ±1.69%
 fs/bench-stat.js statType='fstat' n=200000                                                      -0.14 %       ±0.26% ±0.34% ±0.44%
 fs/bench-stat.js statType='lstat' n=200000                                                      -0.21 %       ±0.26% ±0.35% ±0.45%
 fs/bench-stat.js statType='stat' n=200000                                                       -0.06 %       ±0.29% ±0.38% ±0.49%
 fs/bench-statSync.js statSyncType='fstatSync' n=1000000                                         -0.16 %       ±0.34% ±0.45% ±0.58%
 fs/bench-statSync.js statSyncType='lstatSync' n=1000000                                 ***      7.43 %       ±1.93% ±2.54% ±3.27%
 fs/bench-statSync.js statSyncType='statSync' n=1000000                                  ***      6.65 %       ±2.00% ±2.64% ±3.39%
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType='asc'                    0.16 %       ±0.58% ±0.76% ±0.98%
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType='buf'                    0.16 %       ±0.58% ±0.77% ±0.99%
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType='utf'                    0.37 %       ±0.45% ±0.60% ±0.77%
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType='asc'                 0.29 %       ±0.88% ±1.17% ±1.50%
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType='buf'                 0.80 %       ±0.85% ±1.12% ±1.44%
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType='utf'                -0.05 %       ±0.46% ±0.60% ±0.77%
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType='asc'                    0.57 %       ±0.62% ±0.81% ±1.05%
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType='buf'             *      0.69 %       ±0.59% ±0.77% ±0.99%
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType='utf'                    0.08 %       ±0.32% ±0.42% ±0.54%
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType='asc'                   0.40 %       ±0.43% ±0.56% ±0.72%
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType='buf'                   0.47 %       ±3.25% ±4.29% ±5.52%
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType='utf'                   0.01 %       ±0.21% ±0.28% ±0.36%
 fs/readfile-partitioned.js concurrent=1 len=1024 dur=5                                           0.06 %       ±0.32% ±0.43% ±0.55%
 fs/readfile-partitioned.js concurrent=1 len=16777216 dur=5                                      -0.05 %       ±0.46% ±0.60% ±0.77%
 fs/readfile-partitioned.js concurrent=10 len=1024 dur=5                                         -0.09 %       ±0.22% ±0.29% ±0.38%
 fs/readfile-partitioned.js concurrent=10 len=16777216 dur=5                                     -0.12 %       ±0.24% ±0.31% ±0.40%
 fs/readfile.js concurrent=1 len=1024 dur=5                                                       0.28 %       ±0.34% ±0.44% ±0.57%
 fs/readfile.js concurrent=1 len=16777216 dur=5                                                   0.15 %       ±0.22% ±0.29% ±0.37%
 fs/readfile.js concurrent=10 len=1024 dur=5                                               *      0.36 %       ±0.30% ±0.39% ±0.50%
 fs/readfile.js concurrent=10 len=16777216 dur=5                                                  0.44 %       ±0.59% ±0.78% ±1.00%
 fs/readFileSync.js n=600000                                                             ***      2.50 %       ±0.84% ±1.10% ±1.42%
 fs/write-stream-throughput.js size=1024 encodingType='asc' dur=5                                -0.46 %       ±1.84% ±2.43% ±3.11%
 fs/write-stream-throughput.js size=1024 encodingType='buf' dur=5                                -1.14 %       ±1.90% ±2.51% ±3.23%
 fs/write-stream-throughput.js size=1024 encodingType='utf' dur=5                                -0.30 %       ±1.94% ±2.56% ±3.29%
 fs/write-stream-throughput.js size=1048576 encodingType='asc' dur=5                              1.14 %       ±2.49% ±3.29% ±4.23%
 fs/write-stream-throughput.js size=1048576 encodingType='buf' dur=5                             -0.92 %       ±2.47% ±3.26% ±4.18%
 fs/write-stream-throughput.js size=1048576 encodingType='utf' dur=5                             -1.95 %       ±2.59% ±3.42% ±4.40%
 fs/write-stream-throughput.js size=2 encodingType='asc' dur=5                             *      1.74 %       ±1.49% ±1.96% ±2.52%
 fs/write-stream-throughput.js size=2 encodingType='buf' dur=5                                   -0.02 %       ±0.99% ±1.30% ±1.67%
 fs/write-stream-throughput.js size=2 encodingType='utf' dur=5                                   -0.28 %       ±1.13% ±1.49% ±1.91%
 fs/write-stream-throughput.js size=65535 encodingType='asc' dur=5                                0.09 %       ±2.08% ±2.74% ±3.52%
 fs/write-stream-throughput.js size=65535 encodingType='buf' dur=5                               -0.23 %       ±2.38% ±3.14% ±4.03%
 fs/write-stream-throughput.js size=65535 encodingType='utf' dur=5                                0.28 %       ±1.75% ±2.31% ±2.96%

Looks like there is small performance improvement

@BridgeAR
Copy link
Member

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

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 16, 2018
@BridgeAR
Copy link
Member

@daynin this needs a rebase.

@BridgeAR BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 17, 2018
@daynin
Copy link
Contributor Author

daynin commented Feb 17, 2018

@BridgeAR done

@BridgeAR
Copy link
Member

Light-CI for the rebase https://ci.nodejs.org/job/node-test-commit-light/291/

@daynin
Copy link
Contributor Author

daynin commented Feb 19, 2018

@BridgeAR the conflict was resolved

@daynin
Copy link
Contributor Author

daynin commented Feb 28, 2018 •
edited
Loading

cc @nodejs/collaborators

Hello! Run CI for this PR, please

@gireeshpunathil
Copy link
Member

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

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 2, 2018
@apapirovski
Copy link
Member

Landed in 523d44a

@apapirovski apapirovski closed this Mar 4, 2018
apapirovski pushed a commit that referenced this pull request Mar 4, 2018
@daynin @apapirovski
fs: replace duplicate conditions by function
523d44a 
PR-URL: #18717
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins
Copy link
Contributor

Should this be backported to v9.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.

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
@daynin @BridgeAR
fs: replace duplicate conditions by function
28f3cc2 
PR-URL: nodejs#18717
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@daynin @MayaLekova
fs: replace duplicate conditions by function
4b7b155 
PR-URL: nodejs#18717
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@jasnell
Copy link
Member

jasnell commented Aug 17, 2018

Should this be backported to 8.x? If so, a backport PR will be needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@hiroppy hiroppy hiroppy approved these changes

@richardlau richardlau richardlau approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@iwko iwko iwko approved these changes

@starkwang starkwang starkwang approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet