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: move type checking for multiple fs methods to js #17334

Closed
wants to merge 10 commits into from
Closed

fs: move type checking for multiple fs methods to js #17334

wants to merge 10 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Nov 27, 2017

Update errors to use internal/errors, move type checking

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

fs

@jasnell jasnell added errors Issues and PRs related to JavaScript errors originated in Node.js core. fs Issues and PRs related to the fs subsystem / file system. semver-major PRs that contain breaking changes and should be released in the next major version. labels Nov 27, 2017
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. labels Nov 27, 2017
TimothyGu
TimothyGu approved these changes Nov 27, 2017
View reviewed changes
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

Code LGTM. I'd like a CITGM run though.

maclover7
maclover7 approved these changes Nov 27, 2017
View reviewed changes
Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

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

LGTM. I'm happy to try and tackle it, but it'd be great to try and DRY up some of the validation checks, most seem somewhat similar.

@mscdex
Copy link
Contributor

mscdex commented Nov 27, 2017

How does this affect performance?

addaleax
addaleax approved these changes Nov 27, 2017
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM if benchmarks are good

src/node_file.cc
}

const int64_t len = len_v->IntegerValue();
const int64_t len = args[1]->IntegerValue();
Copy link
Member

Choose a reason for hiding this comment

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

Something something deprecated overload ;)

@jasnell
Copy link
Member Author

jasnell commented Nov 27, 2017 •
edited
Loading

Looking just at fs.fstat, the impact is minimal in the microbenchmarks.

$ ./node benchmark/compare.js --old node --new ./node --filter stat fs > ~/fsbench
[00:32:12|% 100| 2/2 files | 60/60 runs | 3/3 configs]: Done
 james@ubuntu:~/node/node$ cat ~/fsbench | Rscript benchmark/compare.R
                                                         improvement confidence     p.value
 fs/bench-stat.js statType="fstat" n=200000                  -8.78 %            0.489611332
 fs/bench-stat.js statType="lstat" n=200000                  16.78 %          * 0.039583545
 fs/bench-stat.js statType="stat" n=200000                   10.82 %            0.145232808
 fs/bench-statSync.js statSyncType="fstatSync" n=1000000     -2.82 %         ** 0.005483922
 fs/bench-statSync.js statSyncType="lstatSync" n=1000000     -5.78 %         ** 0.001044621
 fs/bench-statSync.js statSyncType="statSync" n=1000000      -1.83 %            0.437744271

(comparing with 9.2.0)

@jasnell
fs: remove unnecessary throw on fs.mkdtemp
4f74917 
The type is already checked in JS. Change to a CHECK
@jasnell jasnell mentioned this pull request Nov 27, 2017
Closed
4 tasks
@jasnell
Copy link
Member Author

jasnell commented Nov 27, 2017

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

@joyeecheung joyeecheung mentioned this pull request Dec 5, 2017
Closed
mcollina
mcollina approved these changes Dec 8, 2017
View reviewed changes
Copy link
Member

@mcollina mcollina 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 it needs a CITGM run.

@jasnell
Copy link
Member Author

jasnell commented Dec 13, 2017

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1150/
New CI: https://ci.nodejs.org/job/node-test-pull-request/12087/

@jasnell
Copy link
Member Author

jasnell commented Dec 13, 2017

Unrelated failures in Linux CI but running again there just to be safe: https://ci.nodejs.org/job/node-test-commit-linux/14937/

jasnell added a commit that referenced this pull request Dec 13, 2017
@jasnell
fs: move type checking for fs.sync to js
8cb080c 
PR-URL: #17334
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
jasnell added a commit that referenced this pull request Dec 13, 2017
@jasnell
fs: move type checking for fs.ftruncate to js
d453fac 
PR-URL: #17334
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
jasnell added a commit that referenced this pull request Dec 13, 2017
@jasnell
fs: move type checking for fs.fchmod to js
0a01aa8 
PR-URL: #17334
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
jasnell added a commit that referenced this pull request Dec 13, 2017
@jasnell
fs: move type checking for fs.fchown to js
82eb459 
PR-URL: #17334
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
jasnell added a commit that referenced this pull request Dec 13, 2017
@jasnell
fs: move type checking in fs.futimes to js
448ec0b 
PR-URL: #17334
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
jasnell added a commit that referenced this pull request Dec 13, 2017
@jasnell
fs: move type checking for fs.read to js
1638698 
PR-URL: #17334
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
jasnell added a commit that referenced this pull request Dec 13, 2017
@jasnell
fs: remove unnecessary throw on fs.mkdtemp
805dca1 
The type is already checked in JS. Change to a CHECK

PR-URL: #17334
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Dec 13, 2017

Landed in 04ae486...805dca1

@jasnell jasnell closed this Dec 13, 2017
@joyeecheung joyeecheung mentioned this pull request May 4, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@refack refack refack approved these changes

@addaleax addaleax addaleax approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@maclover7 maclover7 maclover7 approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. errors Issues and PRs related to JavaScript errors originated in Node.js core. fs Issues and PRs related to the fs subsystem / file system. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@jasnell @mscdex @mcollina @refack @addaleax @TimothyGu @maclover7 @joyeecheung @nodejs-github-bot