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

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
5 participants
@addaleax
Member

addaleax commented Aug 30, 2016

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

View changes

benchmark/fs/bench-stat.js
+
+ bench.start();
+ (function r(cntr) {
+ if (--cntr <= 0)

This comment has been minimized.

@lpinca

lpinca Aug 30, 2016

Member

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

@lpinca

lpinca Aug 30, 2016

Member

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

This comment has been minimized.

@addaleax

addaleax Aug 30, 2016

Member

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

@addaleax

addaleax Aug 30, 2016

Member

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

This comment has been minimized.

addaleax added some commits Aug 30, 2016

benchmark: add benches for fs.stat & fs.statSync
Add very simple benchmarks for `fs.stat` and `fs.statSync` as
well as `fs.lstat` and `fs.lstatSync` based on the `readdir`
benchmarks.
benchmark: fix off-by-one error in fs benchmarks
Fix a off-by-one error that made the benchmarks for asynchronous
functions run `n - 1` times instead of `n` times.
@lpinca

This comment has been minimized.

Show comment
Hide comment
@lpinca

lpinca Aug 30, 2016

Member

LGTM

Member

lpinca commented Aug 30, 2016

LGTM

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 30, 2016

Member

LGTM.
/cc @mscdex

Member

jasnell commented Aug 30, 2016

LGTM.
/cc @mscdex

@jasnell

This comment has been minimized.

Show comment
Hide comment
Member

jasnell commented Sep 9, 2016

ping @mscdex

benchmark/fs/bench-stat.js
+ fn(__filename, function() {
+ r(cntr);
+ });
+ }(n));

This comment has been minimized.

@mscdex

mscdex Sep 9, 2016

Contributor

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

@mscdex

mscdex Sep 9, 2016

Contributor

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

This comment has been minimized.

@addaleax

addaleax Sep 9, 2016

Member

@mscdex Done, if I understood you correctly.

@addaleax

addaleax Sep 9, 2016

Member

@mscdex Done, if I understood you correctly.

benchmark/fs/bench-stat.js
+
+function main(conf) {
+ const n = conf.n >>> 0;
+ const fn = fs[conf.kind];

This comment has been minimized.

@mscdex

mscdex Sep 9, 2016

Contributor

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

@mscdex

mscdex Sep 9, 2016

Contributor

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

This comment has been minimized.

@addaleax

addaleax Sep 9, 2016

Member

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.

@addaleax

addaleax Sep 9, 2016

Member

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

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Sep 9, 2016

Contributor

LGTM

Contributor

mscdex commented Sep 9, 2016

LGTM

jasnell added a commit that referenced this pull request Sep 12, 2016

benchmark: fix off-by-one error in fs benchmarks
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>

jasnell added a commit that referenced this pull request Sep 12, 2016

benchmark: add benches for fs.stat & fs.statSync
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

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Sep 12, 2016

Member

Landed in efabc6a and 450ee63

Member

jasnell commented Sep 12, 2016

Landed in efabc6a and 450ee63

@jasnell jasnell closed this Sep 12, 2016

@addaleax addaleax deleted the addaleax:bench-fs-stat branch Sep 13, 2016

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Sep 13, 2016

Member

@jasnell Thanks for landing this!

Member

addaleax commented Sep 13, 2016

@jasnell Thanks for landing this!

Fishrock123 added a commit that referenced this pull request Sep 14, 2016

benchmark: fix off-by-one error in fs benchmarks
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>

Fishrock123 added a commit that referenced this pull request Sep 14, 2016

benchmark: add benches for fs.stat & fs.statSync
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

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Sep 30, 2016

Member

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

Member

MylesBorins commented Sep 30, 2016

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

MylesBorins added a commit that referenced this pull request Sep 30, 2016

benchmark: add benches for fs.stat & fs.statSync
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 added a commit that referenced this pull request Oct 10, 2016

benchmark: add benches for fs.stat & fs.statSync
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>

rvagg added a commit that referenced this pull request Oct 18, 2016

benchmark: add benches for fs.stat & fs.statSync
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 added a commit that referenced this pull request Oct 26, 2016

benchmark: add benches for fs.stat & fs.statSync
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 MylesBorins referenced this pull request Oct 26, 2016

Closed

V4.6.2 proposal #9298

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment