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

backport: 11665 to v6 #16667

Closed
wants to merge 2 commits into from
Closed

backport: 11665 to v6 #16667

wants to merge 2 commits into from

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Nov 1, 2017

Backported #11665 to v6, should fix #16496.

The fs.realpath*() optimizations is not fully ported since v6 is lack of internal/querystring.

Any feedback is welcome.

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)

@JLHwung
fs: include more fs.stat*() optimizations
b82c613 
Including:

* Move async *stat() functions to FillStatsArray() now used by the
sync *stat() functions

* Avoid creating fs.Stats instances for implicit async/sync *stat()
calls used in various fs functions

* Store reference to Float64Array data on C++ side for easier/faster
access, instead of passing from JS to C++ on every async/sync *stat()
call

Backport-PR-URL: nodejs#11665
Fixes: nodejs#16496
@JLHwung
fs: more realpath*() optimizations
89048c2 
Including:

* Avoid regexp on non-Windows platforms when parsing the root of a path

Backport-PR-URL: nodejs#11665
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. v6.x labels Nov 1, 2017
@JLHwung JLHwung changed the title Backport v6 11665 backport: 11665 to v6 Nov 1, 2017
@mscdex mscdex added fs Issues and PRs related to the fs subsystem / file system. performance Issues and PRs related to the performance of Node.js. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 1, 2017
@MylesBorins MylesBorins closed this Nov 2, 2017
@MylesBorins MylesBorins reopened this Nov 2, 2017
@MylesBorins
Copy link
Contributor

The original PR was Semver-Major, is this backport only semver-patch?

Are there some other patches that might be able to be reverted that could fix this instead of landing the optimization?

Have you tested across various versions of v6.x? Is there a version that works?

Thanks for taking the time to do this port 🎉

@JLHwung JLHwung mentioned this pull request Nov 3, 2017
Closed
2 tasks
@JLHwung
Copy link
Contributor Author

JLHwung commented Nov 3, 2017

The original PR was Semver-Major, is this backport only semver-patch?

This PR is also Semver-Major in the sense that fs.readFileSync now calls fs.fstat instead of fs.fstatSync internally.

Are there some other patches that might be able to be reverted that could fix this instead of landing the optimization?

Yes. See the following section.

Have you tested across various versions of v6.x? Is there a version that works?

I have tested the signedness of dev on many versions of 6.x. The dev signedness of fs.stat, introduced on #8515, is impacted on every version. The signedness of fs.statSync is fixed on #11522 and landed on 6.10.1. But fs.stat is still being impacted.

There is some discussions on the signedness on #8515. So I have worked on another PR to give a hotfix: #16705.

@MylesBorins
Copy link
Contributor

/cc @nodejs/lts can you please take a look

@MylesBorins MylesBorins requested a review from mscdex November 14, 2017 19:14
@MylesBorins
Copy link
Contributor

MylesBorins commented Nov 14, 2017 •
edited
Loading

@mscdex since the original optimizations were done by you, would you be able to review this?
edit: nvm, I see there is a trumping PR

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

@mscdex mscdex Awaiting requested review from mscdex

Assignees
No one assigned
Labels
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. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JLHwung @MylesBorins @mscdex @nodejs-github-bot