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.

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

fs: minor refactoring #1870

Closed
wants to merge 5 commits into from
Closed

Conversation

thefourtheye
Copy link
Contributor

  1. Changing Bad arguments messages to options should either be an object or a string and returning meaningful error message from fs_event_wrap's FSEvent's Start function.
  2. nullCheckCallNT function is not necessary, as we can directly pass callback and er to process.nextTick
  3. We are getting rid of inStatWatchers function and making statWatchers a Map.
  4. In rethrow function, instead of assigning to err and throwing err, we can directly throw backtrace itself.
  5. As SyncWriteStream is only for internal use, it would be better if it is non-enumerable, so that a simple console.log(require('fs')) will not reveal it.

@Fishrock123 Fishrock123 added the fs Issues and PRs related to the fs subsystem / file system. label Jun 2, 2015
@bnoordhuis bnoordhuis force-pushed the master branch 2 times, most recently from 311c3e4 to b926718 Compare June 2, 2015 17:25
@benjamingr
Copy link
Member

Some points:

  • Commit message structure, see how the other commit messages look.
  • Doing Object.create(null) with an in indeed looks better than a hasOwnProperty check and that extra, but I'm not sure if there is any speed difference. The delete also makes sense.

Other than that LGTM.

@thefourtheye
Copy link
Contributor Author

@benjamingr The points I mentioned in the PR would be good enough to be in the commit messages, right?

@benjamingr
Copy link
Member

Right. I think so - check out some other PRs.

statWatchers[filename];
}

var statWatchers = Object.create(null);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe make this const while you're here.

@bnoordhuis
Copy link
Member

@thefourtheye
Copy link
Contributor Author

@benjamingr @bnoordhuis I updated the PR as per the suggestions. Please review.

1. Changing `Bad arguments` error messages to a more helpful message
`options should either be an object or a string`.

2. Made braces consistent.

3. Returning meaningful error message from `fs_event_wrap`'s
`FSEvent`'s `Start` function.
`nullCheckCallNT` function is not necessary, as we can directly pass
`callback` and `er` to `process.nextTick`
@thefourtheye
Copy link
Contributor Author

@benjamingr @bnoordhuis I included another commit which does very minor refactoring stuff. Please let me know if that commit is not worth sending in. I ll pull that out.

@@ -583,11 +580,11 @@ fs.read = function(fd, buffer, offset, length, position, callback) {
};

fs.readSync = function(fd, buffer, offset, length, position) {
var legacy = false;
var legacy = false, encoding;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would prefer encoding to be its own var statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thefourtheye thefourtheye force-pushed the fs-refactoring branch 2 times, most recently from 0f381df to d725865 Compare June 2, 2015 23:14
@thefourtheye
Copy link
Contributor Author

I included a commit which makes SyncWriteStream non-enumerable in fs. Even though it is an undocumented function, it would be better not to reveal it I believe.

@thefourtheye thefourtheye force-pushed the fs-refactoring branch 2 times, most recently from 32d9610 to fcfd07e Compare June 4, 2015 21:28
@thefourtheye
Copy link
Contributor Author

@benjamingr @chrisdickinson @Fishrock123 I wrote a benchmark (this may not be good, please review and give feedback if you have time) and I found that the Map is slightly faster in watchFile and unwatchFile case.

➜  io.js git:(master) ✗ iojs benchmark/fs/stat-watcher.js
fs/stat-watcher.js iteration=1 dur=5000: 27204.23833
fs/stat-watcher.js iteration=2 dur=5000: 27380.47575
fs/stat-watcher.js iteration=3 dur=5000: 26952.29607
fs/stat-watcher.js iteration=4 dur=5000: 27146.52985
fs/stat-watcher.js iteration=5 dur=5000: 26646.59153
fs/stat-watcher.js iteration=6 dur=5000: 25866.48917
fs/stat-watcher.js iteration=7 dur=5000: 27168.83679
fs/stat-watcher.js iteration=8 dur=5000: 26398.98303
fs/stat-watcher.js iteration=9 dur=5000: 27054.75701

➜  io.js git:(master) ✗ git checkout fs-refactoring
...

➜  io.js git:(fs-refactoring) ✗ ./configure; make; sudo make install
...

➜  io.js git:(fs-refactoring) ✗ iojs benchmark/fs/stat-watcher.js 
fs/stat-watcher.js iteration=1 dur=5000: 28733.94148
fs/stat-watcher.js iteration=2 dur=5000: 26745.49034
fs/stat-watcher.js iteration=3 dur=5000: 28694.38873
fs/stat-watcher.js iteration=4 dur=5000: 28190.78273
fs/stat-watcher.js iteration=5 dur=5000: 29029.76751
fs/stat-watcher.js iteration=6 dur=5000: 28882.22880
fs/stat-watcher.js iteration=7 dur=5000: 29025.18686
fs/stat-watcher.js iteration=8 dur=5000: 28236.87782
fs/stat-watcher.js iteration=9 dur=5000: 29540.99272

The first run was from the latest master branch and the second run was in the refactoring branch.

@trevnorris
Copy link
Contributor

I'm not comfortable with the FSReqWrap change. When I implemented that there were nuances about where it needed to be called. I'll want to review that change in more detail.

@thefourtheye
Copy link
Contributor Author

@trevnorris Are you talking about this line? It was just moved up, because inside the if and after the if also we are creating FSReqWrap object.

@trevnorris
Copy link
Contributor

@thefourtheye I see that now. Wrote my first comment from my phone and it wasn't easy to see the changes completely. Okay, LGTM.

@thefourtheye
Copy link
Contributor Author

@trevnorris No problem. Thanks for reviewing :-)

@@ -1112,14 +1114,14 @@ function writeAll(fd, buffer, offset, length, position, callback) {
}

fs.writeFile = function(path, data, options, callback) {
var callback = maybeCallback(arguments[arguments.length - 1]);
callback = maybeCallback(arguments[arguments.length - 1]);
Copy link
Member

Choose a reason for hiding this comment

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

Reading and assigning to arguments at the same time is a deopt.
Not sure if this matters.

Edit: I was wrong, it is optimized in the strict mode, and in sloppy mode it is deoptimized even with a var callback if the variable name is the same as one of the arguments.

Copy link
Member

Choose a reason for hiding this comment

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

it is easy to prevent just by renaming the parameter to something like callback_ and leaving the var keyword where it was.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, there are many places in the file where we do this. So, I am doing this in a commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

even if writing to /dev/null I doubt it could be run enough to notice a difference. and from the looks of it, nothing that way was actually changed. Just removed the unnecessary var.

Copy link
Member

Choose a reason for hiding this comment

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

I tested it (with v8 4.2) and it seems that I was wrong about the deopt, sorry.
'use strict' changes some things related to arguments handling, and it has an effect on the optimization.

@petkaantonov this should be probably mentioned in that example.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ChALkeR I tried something like this

function containsWith(a_) {
    var a = arguments[0] + 1;
}

function containsWith(a) {
    a = arguments[0] + 1;
}

but it still says the function is not optimized for both the cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ChALkeR it's actually a wikipage, you should be able to edit it :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the PR with a commit which will use a different variable name.

Copy link
Member

Choose a reason for hiding this comment

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

@thefourtheye

but it still says the function is not optimized for both the cases.

Have you enabled 'use strict' for the file or for the function?

function unoptimized0(a, b) {
    b = arguments[arguments.length - 1] + 1;
    return b * 1;
}
function unoptimized1(a, b) {
    var b = arguments[arguments.length - 1] + 1;
    return b * 1;
}
function optimized0(a, b) {
    'use strict';
    b = arguments[arguments.length - 1] + 1;
    return b * 1;
}
function optimized1(a, b) {
    'use strict';
    var b = arguments[arguments.length - 1] + 1;
    return b * 1;
}

Edit: There was an error in the last function name.

@@ -91,16 +94,12 @@ function nullCheck(path, callback) {
er.code = 'ENOENT';
if (typeof callback !== 'function')
throw er;
process.nextTick(nullCheckCallNT, callback, er);
process.nextTick(callback, er);
Copy link
Contributor

Choose a reason for hiding this comment

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

glad there are others around to clean up my mess. thanks for catching it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@trevnorris Got your back ;-)

throw new TypeError('Bad arguments');
} else if (typeof options !== 'object') {
throwOptionsError(options);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like the {} were added for stylistic reasons?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@trevnorris Yup, when I was preparing the patch, I am not sure why, but linter was complaining about throwOptionsError being in that position. I had to use {} to fix it. Now, linter is fine without {}. Shall I revert it?

Copy link
Member

Choose a reason for hiding this comment

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

@thefourtheye it is probably this eslint bug again. We already encountered it in #1742

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@targos Yup, that looks more like it. But, I just locally ran make lint without the braces and it is fine. Don't know why. I think its better to leave it as it is now.

@trevnorris
Copy link
Contributor

think the final list of commits should be:

  • Changes in error messages
  • New use of Map
  • nextTick() cleanup and making SyncWriteStream not enumerable
  • All other style fixes

If can do that if you'd like, or you can take care of it. Either way, LGTM.

@thefourtheye
Copy link
Contributor Author

@trevnorris I squashed the commits and edited the commit messages a little bit. Please have a look at them.

@trevnorris
Copy link
Contributor

Looks great. Just want to run CI one last time for sanity sake then I'll land these.

https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/797/

@trevnorris
Copy link
Contributor

There were a total of 6 failures (1 on armv7-wheezy, 5 on win2008r2). None of them look related to this patch. Going to land.

trevnorris pushed a commit to trevnorris/node that referenced this pull request Jun 10, 2015
1. Change "Bad arguments" error messages to a more helpful message
"options should either be an object or a string".

2. Make braces consistent.

3. Return meaningful error message from fs_event_wrap's
FSEvent's Start function.

PR-URL: nodejs#1870
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
trevnorris pushed a commit to trevnorris/node that referenced this pull request Jun 10, 2015
`nullCheckCallNT()` function is not necessary, as we can directly pass
`callback` and `er` to `process.nextTick()`.

PR-URL: nodejs#1870
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
trevnorris pushed a commit to trevnorris/node that referenced this pull request Jun 10, 2015
Remove `inStatWatchers` function and make `statWatchers` a `Map`.

PR-URL: nodejs#1870
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
trevnorris pushed a commit to trevnorris/node that referenced this pull request Jun 10, 2015
1. Remove a few unnecessary variables to reduce LoC.

2. Remove redundant `var` definitions of variables in same function.

3. Refactor variables which are defined inside a block and used outside
as well.

4. Refactor effect-less code.

5. In `rethrow` function, instead of assigning to `err` and throwing
`err` directly throw `backtrace` object.

6. Reassign a defined parameter while also mentioning arguments in the
body is one of the optimization killers. So, changing `callback` to
`callback_` and declaring a new variable called `callback` in the body.

PR-URL: nodejs#1870
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
trevnorris pushed a commit to trevnorris/node that referenced this pull request Jun 10, 2015
Make SyncWriteStream non-enumerable since it's only for internal use.

PR-URL: nodejs#1870
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
@trevnorris
Copy link
Contributor

Landed in 09f2a67, 67a11b9, 8841132, a011c32 and 53a4eb3 with a few commit message grammatical changes. Thanks much for the work.

Quick note, it's preferred to use the present in commit messages. e.g. "Change parameters" instead of "Changing parameters". Also preferred to not use personal pronouns. e.g. "New function foo() can return value x" instead of "We can return the value x from the new function foo()".

@trevnorris trevnorris closed this Jun 10, 2015
We are getting rid of `inStatWatchers` function and making
`statWatchers` a `Map`.
1. Removed a few unnecessary variables to reduce LoC

2. Removed redundant `var` definitions of variables in same function

3. Refactored variables which are defined inside a block and used
outside as well

4. Refactored effect-less code

5. In `rethrow` function, instead of assigning to `err` and throwing
`err`, we can directly throw `backtrace` object itself.

6. Reassigning a defined parameter while also mentioning arguments in
the body is one of the optimization killers. So, changing `callback` to
`callback_` and declaring a new variable called `callback` in the body.
As `SyncWriteStream` is only for internal use, it would be better
if it is non-enumerable, so that a simple `console.log(require('fs'))`
will not reveal it.
@thefourtheye
Copy link
Contributor Author

@trevnorris Thanks :) I ll remember the suggestions for the next time :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants