fs: refactor fs for performance #15476
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Also change a few var to const
The main changes are - combine errorHandler and nullCheck - remove getOptions and copyObject - improve callback handling - improve flags handling - improve writeFile and instanceOf checks - improve read- and writestream - improve path check - inline functions if possible - var to const The code got also cleaned up in the process
nodejs-github-bot
added
the
lib / src
BridgeAR
added
the
performance
Member
|
Great to see this but just a heads up, there are likely to be some conflicts with the fs-promises PR I'm working on currently (looking to open that PR either tomorrow or thursday). I'll ping you when I do. |
refack
approved these changes
Sep 23, 2017
Member
Author
|
Since fs has significantly changed since this PR and I do not have the time to pick this up anytime soon, I am going to close it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a alternative to #12105 by @mscdex. I copied some of his stream changes - so those attributions go to @mscdex!
It does not depend on any data copying and is not blocked therefore. The benchmarks I ran against that PR were either on par or better, even though the numbers are not huge due to the v8 changes since that PR was opened.
I still have to add and run more benchmarks but I wanted to have some eyes on this already.
In general the only change that could be considered breaking is the function inlining and the direct usage of the bindings. Therefore monkey patching (as e.g. done in some tests that I removed for now) is not possible anymore. Please note that some functions used bindings directly before and some did not! I split those changes in individual commits. So I can also split this PR into multiple ones but I wanted to have a single one with all changes for the results.
One question is if we want to keep the bindings monkey patchable for our tests or not.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
fs, streams, buffer