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: add --format csv option #7961

Closed
wants to merge 3 commits into from
Closed

benchmark: add --format csv option #7961

wants to merge 3 commits into from

Conversation

adrian-nitu-92
Copy link
Contributor

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

benchmark

Description of change

Added the option of using --format csv when outputting data.

@nodejs-github-bot nodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label Aug 3, 2016
@AndreasMadsen
Copy link
Member

I don't think we want to add extra arguments to each individual benchmark. That is a lot of extra complexity in parsing and output and it is not needed since benchmark/run.js or benchmark/scatter.js can be used to get the same result. Modifying only benchmark/run.js should be sufficient, I'm sorry if that wasn't clear.

@jasnell
Copy link
Member

jasnell commented Aug 3, 2016

+1 to what @AndreasMadsen is saying here. I think there's an easier way to do this.

@adrian-nitu-92
Copy link
Contributor Author

Fair point :), reduced the change set.
I added a secondary commit that fixes some typos and adds a set of missing {}.

`, {
arrayArgs: ['set']
});
const benchmarks = cli.benchmarks();
const validFormats = ['csv', 'default'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be 'simple'?

Copy link
Contributor Author

@adrian-nitu-92 adrian-nitu-92 Aug 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could also be removed :))), but I wanted to add something to not let 'csv' alone :D. Anything should be good, it is an option I doubt would be used. Should I make the change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh? I meant, in the usage text you added --format as simple/csv, but this array doesn’t seem to match that. Sorry if that’s been unclear (or I’m just misunderstanding this in general?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yea, my bad, thank you for spotting the inconsistency :D

@AndreasMadsen
Copy link
Member

AndreasMadsen commented Aug 4, 2016

@adrian-nitu-92 Yes, this is much better. I've sugested some changes, you will also have to:

  • remove console.log(filename); in the csv case.
  • add csv header: console.log('"filename", "configuration", "rate", "time"'); should do.

edit: could you @intel.com email to your GitHub account. I don't know if we require the commits to link to your user account, but it would be nice.

@adrian-nitu-92
Copy link
Contributor Author

adrian-nitu-92 commented Aug 4, 2016

Updated the PR with all changes, except removing the variable validFormats :)

(function recursive(i) {
const filename = benchmarks[i];
const child = fork(path.resolve(__dirname, filename), cli.optional.set);

console.log();
console.log(filename);
if (format == 'csv') {
console.log('"filename","configuration","rate","time"');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to output a csv header for every file. A csv header should only appear once.

@adrian-nitu-92
Copy link
Contributor Author

Updated PR :)

@AndreasMadsen
Copy link
Member

Great. Be sure to run make lint, I did see at least one lint error.

@AndreasMadsen
Copy link
Member

return;
}

if (format == 'csv') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Trott Does this really match our lint rules? The linter says it does. https://ci.nodejs.org/job/node-test-linter/3613/console

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, as far as I can tell, that is consistent with our lint rules. (I would prefer this be === rather than == but that's not lint-enforced.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So would I. @adrian-nitu-92 could you fix this, I think there is another one too.

@AndreasMadsen
Copy link
Member

/cc @nodejs/benchmarking @addaleax @mscdex who participated in the original issue. I need to run this to be sure (can't right now), but besides a "lint" nit this looks good.

Trott added a commit to Trott/io.js that referenced this pull request Aug 6, 2016
@Trott Trott mentioned this pull request Aug 6, 2016
2 tasks
@adrian-nitu-92
Copy link
Contributor Author

Ou, I get it now :D. Made the requested changes :)

@@ -10,31 +10,54 @@ const cli = CLI(`usage: ./node run.js [options] [--] <category> ...

--filter pattern string to filter benchmark scripts
--set variable=value set benchmark variable (can be repeated)
--format [simple|csv] optional value that specifies the output format
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you align the descriptions. ``optional => optional

@AndreasMadsen
Copy link
Member

@adrian-nitu-92 awesome work. I have tried it out and it looks good.

As I'm not a very experienced node.js reviewer, thus I will give this 48 hours before merging, such other contributors have time to come with last remarks.

@jasnell
Copy link
Member

jasnell commented Aug 8, 2016

LGTM


console.log(`${data.name}${conf}: ${data.rate}`);
// delete first space of the configuration
conf = conf.slice(1);
Copy link
Contributor Author

@adrian-nitu-92 adrian-nitu-92 Aug 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line can be removed if I reformat 57&59. I like this version better because I don't think you should rely on spaces in strings you are concatenating.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, just keep it.

Trott added a commit to Trott/io.js that referenced this pull request Aug 9, 2016
Refs: nodejs#7961 (comment)
PR-URL: nodejs#8000
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
Refs: #7961 (comment)
PR-URL: #8000
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
@AndreasMadsen
Copy link
Member

AndreasMadsen commented Aug 11, 2016

LGTM

CI: https://ci.nodejs.org/job/node-test-pull-request/3621/

will merge, when ci is green.


const validFormats = ['csv', 'simple'];
const format = cli.optional.format || 'simple';
if (!validFormats.includes(cli.optional.format)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be !validFormats.includes(format) otherwise it will print Invalid format detected when there are no --format argument.

@AndreasMadsen
Copy link
Member

I found a bug, I will merge when that gets fixed.

I noticed some typos and the lack of {} following an if.

Signed-off-by: Adrian Nitu <[email protected]>
Node documentation recommends using process.exitCode = x and returning
as a way to exit.

Signed-off-by: Adrian Nitu <[email protected]>
Added the option of using --format csv when outputting data.

Signed-off-by: Adrian Nitu <[email protected]>
@adrian-nitu-92
Copy link
Contributor Author

Updated the PR and the branch merging point ( pulled nodejs latest and rebased on top of it, no conflicts detected). Also ran the whole tests again just to make sure I don't have additional slip-ups :D

Thanks Andreas for your help :D

@AndreasMadsen
Copy link
Member

Thanks @adrian-nitu-92. Landed in 9e7fd8e 4b527a4 474e629

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants