perf(cd): only run stat once

[ariporad]

This increases performance somewhat

[ariporad]

@nfischer: Ready for Review.

[nfischer]

Just tested this out, and it looks like both common.error() statements get run if the user types something like cd('file.txt') (where file.txt is a regular file that exists). Could we fix this?

[ariporad]

Oh, yeah. I'll fix that.

[nfischer]

Tried this out. I saw a slight perf win, which is awesome! I just have one comment above about correctness of error messages. If that gets fixed, I can merge

[nfischer]

Also, just tried this, and it has the output Error: cd: no such file or directory: file.txt if I set('-e'), which is also not the proper output.

[ariporad]

@nfischer: What did you run exactly?

[nfischer]

@ariporad

Here's what I ran:

$ cat script1.js
#!/usr/bin/env node
require('shelljs/global');

touch('file.txt');
set('-e');
cd('file.txt');

$ node script1.js
/home/nate/PR_367/node_modules/shelljs/src/common.js:248
        throw e;
        ^

Error: cd: no such file or directory: file.txt
    at Object.error (/home/nate/PR_367/node_modules/shelljs/src/common.js:42:11)
    at _cd (/home/nate/PR_367/node_modules/shelljs/src/cd.js:29:12)
    ... (there's more lines of the stack trace)

$ cat script2.js
#!/usr/bin/env node
require('shelljs/global');

touch('file.txt');
config.silent = true;
cd('.');
echo(error());
cd('fakedirectoryname');
echo(error());
cd('file.txt');
echo(error());

$ node script2.js
null
cd: no such file or directory: fakedirectoryname
cd: not a directory: file.txt
cd: no such file or directory: file.txt

[nfischer]

@ariporad pinging this to see if the issue with error messages has been fixed yet.

[ariporad]

@nfischer: I'm not experiencing the behavior that you're describing. I tried both your scripts, they work as expected for me. (The tests are also passing).

[nfischer]

I'll try again today. The issue is with stderr output, not the value of error() (only error() is checked, not the actual output, in the tests)

[nfischer]

@ariporad I just updated the code a bit to improve performance a bit more. We should rename this PR to "only run stat never 👍".

It turns out you were right. The incorrect error message isn't due to your code, it's due to a bug that was introduced in #357. I'm currently trying to work out a fix for this (it shouldn't be too hard). Expect to see the fix sometime tonight.

Here's some perf stats I saw on my machine. This is the script I ran (named cd.js):

require('shelljs/global');
for (var k=0; k<20000; k++) {
  cd('dir/'); // this is a directory that exists
  cd('..');
}

Here's the perf I saw:

# for the original implementation
$ time -p node cd.js
real 2.61
user 2.50
sys 0.13

# for your revision
$ time -p node cd.js
real 2.11
user 1.98
sys 0.15

# for my revision
$ time -p node cd.js
real 1.96
user 1.90
sys 0.07

I also tested things with cd'ing into a file and into a non-existent directory. I saw perf wins for my revision there as well, but this is mainly designed to optimize for the common case. I don't think performance is as big of a concern for when the command errors-out (and with config.fatal, it isn't a concern at all).

Merge at will.

[nfischer]

Update: I thought a bit more about the error message, and I decided that it's probably easier to fix the issue here than elsewhere. So I updated the PR with the fix. This will now only output the proper error message.

Perf stats are still the same.

[ariporad]

@nfischer: LGTM, but one thing just occurred to me: should we eliminate tey/catch statements? Might be faster (http://git.io/fast-v8)

[nfischer]

@ariporad I think try-catch may be necessary here. I know it adds some performance overhead, but statSync() throws an error if the file doesn't exist (and the alternative function is deprecated). I think we should just stick with this. We'll never beat Bash's cd (it's a shell builtin written in C), but this is probably good enough.

[ariporad]

Ok