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

benchmark: fix fs\bench-realpathSync.js #11904

Closed
wants to merge 1 commit into from
Closed

benchmark: fix fs\bench-realpathSync.js #11904

wants to merge 1 commit into from

Conversation

vsemozhetbyt
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

benchmark, fs

fs\bench-realpathSync.js runs safely only if its cwd is its __dirname. In other cases, it throws ENOENT: no such file or directory. This commit makes it callable from any place (including being a part of the fs suite launched from any directory).

Make it call-site-cwd-independent.
@nodejs-github-bot nodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label Mar 17, 2017
@mscdex
Copy link
Contributor

mscdex commented Mar 17, 2017

I've been running that benchmark quite a bit lately and haven't had problems running it outside of the benchmark/fs/ directory, including via benchmark/compare.js. How are you triggering the error exactly?

@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Mar 17, 2017
@vsemozhetbyt
Copy link
Contributor Author

@mscdex

J:\temp\_git\node-fork\benchmark\fs>bench-realpathSync.js
fs\bench-realpathSync.js type="relative" n=10000: 4,399.895823842575
fs\bench-realpathSync.js type="resolved" n=10000: 4,541.6802584585585
J:\temp>j:\temp\_git\node-fork\benchmark\fs\bench-realpathSync.js
fs.js:960
  binding.lstat(pathModule._makeLong(path), statValues);
          ^

Error: ENOENT: no such file or directory, lstat 'J:\lib'
    at Object.fs.lstatSync (fs.js:960:11)
    at Object.realpathSync (fs.js:1630:21)
    at relativePath (J:\temp\_git\node-fork\benchmark\fs\bench-realpathSync.js:31:8)
    at main (J:\temp\_git\node-fork\benchmark\fs\bench-realpathSync.js:21:5)
    at Benchmark.process.nextTick (J:\temp\_git\node-fork\benchmark\common.js:34:28)
    at _combinedTickCallback (internal/process/next_tick.js:73:7)
    at process._tickCallback (internal/process/next_tick.js:104:9)
    at Module.runMain (module.js:607:11)
    at run (bootstrap_node.js:425:7)
    at startup (bootstrap_node.js:146:9)

@vsemozhetbyt
Copy link
Contributor Author

Also:

J:\temp>node _git\node-fork\benchmark\compare.js --old node.v8-5.9.exe --new node.v8-5.9.turbo-on.exe --filter bench-realpathSync.js fs > bm.csv
[00:00:01|%   0| 0/1 files |  0/60 runs | 0/2 configs]: fs\bench-realpathSync.jsfs.js:878
  binding.lstat(pathModule._makeLong(path), statValues);
          ^

Error: ENOENT: no such file or directory, lstat 'J:\lib'
    at Object.fs.lstatSync (fs.js:878:11)
    at Object.realpathSync (fs.js:1548:21)
    at relativePath (J:\temp\_git\node-fork\benchmark\fs\bench-realpathSync.js:31:8)
    at main (J:\temp\_git\node-fork\benchmark\fs\bench-realpathSync.js:21:5)
    at Benchmark.process.nextTick (J:\temp\_git\node-fork\benchmark\common.js:34:28)
    at _combinedTickCallback (internal/process/next_tick.js:73:7)
    at process._tickCallback (internal/process/next_tick.js:104:9)
    at Module.runMain (module.js:607:11)
    at run (bootstrap_node.js:425:7)
    at startup (bootstrap_node.js:146:9)

@vsemozhetbyt
Copy link
Contributor Author

@vsemozhetbyt
Copy link
Contributor Author

@mscdex Is it OK to land this or should I provide more info?

@mscdex
Copy link
Contributor

mscdex commented Mar 21, 2017

It's fine by me I guess.

@vsemozhetbyt
Copy link
Contributor Author

Landed in ab2d49b

vsemozhetbyt added a commit that referenced this pull request Mar 21, 2017
Make it call-site-cwd-independent.

PR-URL: #11904
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@vsemozhetbyt vsemozhetbyt deleted the bench-realpathSync branch March 21, 2017 12:07
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
Make it call-site-cwd-independent.

PR-URL: nodejs#11904
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@seishun
Copy link
Contributor

seishun commented Mar 21, 2017

Not a big thing, but I'd just like to note for future reference that forward slashes are preferable in paths.

@vsemozhetbyt
Copy link
Contributor Author

@seishun Thank you. Sorry.

MylesBorins pushed a commit that referenced this pull request Mar 28, 2017
Make it call-site-cwd-independent.

PR-URL: #11904
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Mar 28, 2017
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
2 tasks
@MylesBorins
Copy link
Contributor

MylesBorins commented Apr 18, 2017

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace do-not-land if it is being backported

edit: nvm it lands 🎉

MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
Make it call-site-cwd-independent.

PR-URL: #11904
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
Make it call-site-cwd-independent.

PR-URL: #11904
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
Make it call-site-cwd-independent.

PR-URL: nodejs/node#11904
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants