benchmark: add benches for fs.stat & fs.statSync by addaleax · Pull Request #8338 · nodejs/node

addaleax · 2016-08-30T12:59:48Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

benchmark

Description of change

Add very simple benchmarks for fs.stat and fs.statSync as well as fs.lstat and fs.lstatSync based on the readdir benchmarks.

Linter run: https://ci.nodejs.org/job/node-test-linter/4005/

lpinca · 2016-08-30T14:42:12Z

benchmark/fs/bench-stat.js

Nit: doesn't this make the benchmark run n - 1 times?

You’re right. I’ll fix this in the other tests, too.

@lpinca done!

Fix a off-by-one error that made the benchmarks for asynchronous functions run `n - 1` times instead of `n` times.

Add very simple benchmarks for `fs.stat` and `fs.statSync` as well as `fs.lstat` and `fs.lstatSync` based on the `readdir` benchmarks.

lpinca · 2016-08-30T15:46:05Z

LGTM

jasnell · 2016-08-30T15:50:24Z

LGTM.
/cc @mscdex

jasnell · 2016-09-09T16:24:26Z

ping @mscdex

mscdex · 2016-09-09T17:11:37Z

benchmark/fs/bench-stat.js

+    fn(__filename, function() {
+      r(cntr);
+    });
+  }(n));


Perhaps we should be passing fn into the closure as well?

@mscdex Done, if I understood you correctly.

mscdex · 2016-09-09T18:33:19Z

benchmark/fs/bench-stat.js

+
+function main(conf) {
+  const n = conf.n >>> 0;
+  const fn = fs[conf.kind];


I don't know how much it matters/affects things, but maybe we should change this variable name to avoid shadowing?

I’ve simply replaced the only use of this variable with fs[conf.kind]. It doesn’t seem to make any difference, and I would expect any difference it makes to be pretty weak in comparison to the cost of the actual syscall/thread pool operation.

mscdex · 2016-09-09T18:45:01Z

LGTM

Fix a off-by-one error that made the benchmarks for asynchronous functions run `n - 1` times instead of `n` times. PR-URL: #8338 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

Add very simple benchmarks for `fs.stat` and `fs.statSync` as well as `fs.lstat` and `fs.lstatSync` based on the `readdir` benchmarks. PR-URL: #8338 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

jasnell · 2016-09-12T03:54:48Z

Landed in efabc6a and 450ee63

addaleax · 2016-09-13T10:41:24Z

@jasnell Thanks for landing this!

Fix a off-by-one error that made the benchmarks for asynchronous functions run `n - 1` times instead of `n` times. PR-URL: #8338 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

Add very simple benchmarks for `fs.stat` and `fs.statSync` as well as `fs.lstat` and `fs.lstatSync` based on the `readdir` benchmarks. PR-URL: #8338 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

MylesBorins · 2016-09-30T22:26:06Z

This does not land cleanly in LTS. Added dont-land label. Please feel free to manually backport

Add very simple benchmarks for `fs.stat` and `fs.statSync` as well as `fs.lstat` and `fs.lstatSync` based on the `readdir` benchmarks. PR-URL: #8338 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

addaleax added fs Issues and PRs related to the fs subsystem / file system. benchmark Issues and PRs related to the benchmark subsystem. labels Aug 30, 2016

lpinca reviewed Aug 30, 2016
View reviewed changes

addaleax added 2 commits August 30, 2016 17:16

benchmark: fix off-by-one error in fs benchmarks

58f5e9b

Fix a off-by-one error that made the benchmarks for asynchronous functions run `n - 1` times instead of `n` times.

benchmark: add benches for fs.stat & fs.statSync

82de541

Add very simple benchmarks for `fs.stat` and `fs.statSync` as well as `fs.lstat` and `fs.lstatSync` based on the `readdir` benchmarks.

addaleax force-pushed the bench-fs-stat branch from bff12fa to 82de541 Compare August 30, 2016 15:17

mscdex reviewed Sep 9, 2016
View reviewed changes

Pass used function directly to the closure

78608a0

mscdex reviewed Sep 9, 2016
View reviewed changes

Maybe? simplify function call/remove variable shadowing

514a8a7

jasnell closed this Sep 12, 2016

addaleax deleted the bench-fs-stat branch September 13, 2016 10:40

MylesBorins added the dont-land-on-v4.x label Sep 30, 2016

MylesBorins mentioned this pull request Oct 26, 2016

V4.6.2 proposal #9298

Closed

Uh oh!

Conversation

addaleax commented Aug 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

lpinca Aug 30, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax Aug 30, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax Aug 30, 2016

Choose a reason for hiding this comment

Uh oh!

lpinca commented Aug 30, 2016

Uh oh!

jasnell commented Aug 30, 2016

Uh oh!

jasnell commented Sep 9, 2016

Uh oh!

mscdex Sep 9, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax Sep 9, 2016

Choose a reason for hiding this comment

Uh oh!

mscdex Sep 9, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax Sep 9, 2016

Choose a reason for hiding this comment

Uh oh!

mscdex commented Sep 9, 2016

Uh oh!

jasnell commented Sep 12, 2016

Uh oh!

addaleax commented Sep 13, 2016

Uh oh!

MylesBorins commented Sep 30, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

addaleax commented Aug 30, 2016 •

edited

Loading