fs: minor refactoring #1870

thefourtheye · 2015-06-02T17:20:42Z

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.
nullCheckCallNT function is not necessary, as we can directly pass callback and er to process.nextTick
We are getting rid of inStatWatchers function and making statWatchers a Map.
In rethrow function, instead of assigning to err and throwing err, we can directly throw backtrace itself.
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.

benjamingr · 2015-06-02T19:41:13Z

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 · 2015-06-02T19:44:25Z

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

benjamingr · 2015-06-02T19:48:20Z

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

bnoordhuis · 2015-06-02T19:48:46Z

lib/fs.js

-      statWatchers[filename];
-}
-
+var statWatchers = Object.create(null);


Maybe make this const while you're here.

bnoordhuis · 2015-06-02T19:50:16Z

LGTM with a suggestion. CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/755/

thefourtheye · 2015-06-02T20:04:24Z

@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 · 2015-06-02T22:23:00Z

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

chrisdickinson · 2015-06-02T22:25:10Z

lib/fs.js

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


Would prefer encoding to be its own var statement.

@chrisdickinson Done.

thefourtheye · 2015-06-03T15:29:06Z

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 · 2015-06-04T21:38:22Z

@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 · 2015-06-05T17:16:57Z

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 · 2015-06-05T17:24:21Z

@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 · 2015-06-05T17:30:35Z

@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 · 2015-06-05T18:42:54Z

@trevnorris No problem. Thanks for reviewing :-)

ChALkeR · 2015-06-10T16:44:12Z

lib/fs.js

@@ -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]);


~~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.

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

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

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.

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.

@thefourtheye I'm using this test: petkaantonov/bluebird/wiki/Optimization-killers#1-tooling with recent iojs.

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

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

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

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

trevnorris · 2015-06-10T17:39:12Z

lib/fs.js

@@ -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);


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

@trevnorris Got your back ;-)

trevnorris · 2015-06-10T17:42:56Z

lib/fs.js

-    throw new TypeError('Bad arguments');
+  } else if (typeof options !== 'object') {
+    throwOptionsError(options);
+  }


looks like the {} were added for stylistic reasons?

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

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

@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 · 2015-06-10T18:40:36Z

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 · 2015-06-10T18:52:42Z

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

trevnorris · 2015-06-10T20:35:16Z

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 · 2015-06-10T22:49:51Z

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.

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]>

`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]>

Remove `inStatWatchers` function and make `statWatchers` a `Map`. PR-URL: nodejs#1870 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Trevor Norris <[email protected]>

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]>

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 · 2015-06-10T23:08:13Z

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()".

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 · 2015-06-11T02:10:07Z

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

Fishrock123 added the fs Issues and PRs related to the fs subsystem / file system. label Jun 2, 2015

bnoordhuis force-pushed the master branch 2 times, most recently from 311c3e4 to b926718 Compare June 2, 2015 17:25

bnoordhuis reviewed Jun 2, 2015
View reviewed changes

lib/fs.js

statWatchers[filename];

}

var statWatchers = Object.create(null);

Copy link

Member

bnoordhuis Jun 2, 2015

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.

thefourtheye force-pushed the fs-refactoring branch from c18484f to 40d4c54 Compare June 2, 2015 20:02

thefourtheye force-pushed the fs-refactoring branch from 40d4c54 to 5a5fc58 Compare June 2, 2015 20:25

fs: better error messages

a087ed9

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.

thefourtheye force-pushed the fs-refactoring branch from 5a5fc58 to de2a7f1 Compare June 2, 2015 21:09

fs: removing unnecessary nullCheckCallNT

f53c082

`nullCheckCallNT` function is not necessary, as we can directly pass `callback` and `er` to `process.nextTick`

chrisdickinson reviewed Jun 2, 2015
View reviewed changes

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

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

ChALkeR reviewed Jun 10, 2015
View reviewed changes

thefourtheye force-pushed the fs-refactoring branch from f6cb789 to 858ee0d Compare June 10, 2015 17:20

trevnorris reviewed Jun 10, 2015
View reviewed changes

thefourtheye force-pushed the fs-refactoring branch from 858ee0d to 365bc72 Compare June 10, 2015 18:51

thefourtheye mentioned this pull request Jun 10, 2015

Release proposal: 2.3.0 #1939

Closed

trevnorris closed this Jun 10, 2015

thefourtheye added 3 commits June 11, 2015 00:18

fs: Removing inStatWatchers & using Map for lookup

73d011e

We are getting rid of `inStatWatchers` function and making `statWatchers` a `Map`.

fs: Making SyncWriteStream non-enumerable

365bc72

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 deleted the fs-refactoring branch June 11, 2015 02:12

This was referenced Jun 22, 2023

[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#506

Open

[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#508

Open

[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#543

Open

[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#574

Open

This was referenced Jul 7, 2023

[Snyk] Fix for 4 vulnerabilities leonardoadame/node#63

Open

[Snyk] Fix for 9 vulnerabilities leonardoadame/node#128

Open

fs: minor refactoring #1870

fs: minor refactoring #1870

Conversation

thefourtheye commented Jun 2, 2015

benjamingr commented Jun 2, 2015

thefourtheye commented Jun 2, 2015

benjamingr commented Jun 2, 2015

Choose a reason for hiding this comment

bnoordhuis commented Jun 2, 2015

thefourtheye commented Jun 2, 2015

thefourtheye commented Jun 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thefourtheye commented Jun 3, 2015

thefourtheye commented Jun 4, 2015

trevnorris commented Jun 5, 2015

thefourtheye commented Jun 5, 2015

trevnorris commented Jun 5, 2015

thefourtheye commented Jun 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevnorris commented Jun 10, 2015

thefourtheye commented Jun 10, 2015

trevnorris commented Jun 10, 2015

trevnorris commented Jun 10, 2015

trevnorris commented Jun 10, 2015

thefourtheye commented Jun 11, 2015