Skip to content

backport: 11665 to v6#16667

Closed
JLHwung wants to merge 2 commits intonodejs:v6.x-stagingfrom
JLHwung:backport-v6-11665
Closed

backport: 11665 to v6#16667
JLHwung wants to merge 2 commits intonodejs:v6.x-stagingfrom
JLHwung:backport-v6-11665

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

MylesBorins commented Nov 2, 2017

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

MylesBorins commented Nov 3, 2017

/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