Skip to content

Conversation

@jasnell
Copy link
Member

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

@jasnelljasnell 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-botnodejs-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
Copy link
Member

@TimothyGuTimothyGu 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.

Copy link
Contributor

@maclover7maclover7 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

How does this affect performance?

Copy link
Member

@addaleaxaddaleax 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

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
MemberAuthor

jasnell commented Nov 27, 2017

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)

@jasnelljasnellforce-pushed the fs-migrate-internal-errors branch from 88f7a36 to 458ab44CompareNovember 27, 2017 04:51
The type is already checked in JS. Change to a CHECK
@jasnell
Copy link
MemberAuthor

Copy link
Member

@mcollinamcollina 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
MemberAuthor

@jasnell
Copy link
MemberAuthor

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
PR-URL: #17334 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
jasnell added a commit that referenced this pull request Dec 13, 2017
PR-URL: #17334 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
jasnell added a commit that referenced this pull request Dec 13, 2017
PR-URL: #17334 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
jasnell added a commit that referenced this pull request Dec 13, 2017
PR-URL: #17334 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
jasnell added a commit that referenced this pull request Dec 13, 2017
PR-URL: #17334 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
jasnell added a commit that referenced this pull request Dec 13, 2017
PR-URL: #17334 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
jasnell added a commit that referenced this pull request Dec 13, 2017
The type is already checked in JS. Change to a CHECK PR-URL: #17334 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
@jasnell
Copy link
MemberAuthor

Landed in 04ae486...805dca1

Sign up for freeto 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++.errorsIssues and PRs related to JavaScript errors originated in Node.js core.fsIssues and PRs related to the fs subsystem / file system.semver-majorPRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@jasnell@mscdex@mcollina@refack@addaleax@TimothyGu@maclover7@joyeecheung@nodejs-github-bot