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.

Sign up for GitHub

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

Jump to bottom

benchmark: improve compare output #18597

Closed
wants to merge 2 commits into from
Closed

benchmark: improve compare output #18597

wants to merge 2 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Feb 6, 2018

The current output uses JSON.stringify to escape the config values.
This switches to util.inspect to have a better readable output.

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

benchmark

@BridgeAR
benchmark: improve compare output
ff1ac36 
The current output uses JSON.stringify to escape the config values.
This switches to util.inspect to have a better readable output.
@nodejs-github-bot nodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label Feb 6, 2018
AndreasMadsen
AndreasMadsen reviewed Feb 6, 2018
View reviewed changes
benchmark/compare.js Outdated
}
conf = conf.slice(1);
// Escape quotes (") for correct csv formatting
conf = conf.replace(/"/g, '""');
Copy link
Member

@AndreasMadsen AndreasMadsen Feb 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

How does using inspect prevent quotes completely from appearing? Could data.conf[key] not contain a " itself?

Copy link
Member

Choose a reason for hiding this comment

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

Seconding this question.

Copy link
Member Author

Choose a reason for hiding this comment

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

Uh, you are right. I was a bit hasty here. I revert that change.

apapirovski
apapirovski reviewed Feb 6, 2018
View reviewed changes
benchmark/compare.js
@@ -76,11 +77,9 @@ if (showProgress) {
// Construct configuration string, " A=a, B=b, ..."
let conf = '';
for (const key of Object.keys(data.conf)) {
conf += ` ${key}=${JSON.stringify(data.conf[key])}`;
conf += ` ${key}=${inspect(data.conf[key])}`;
Copy link
Member

Choose a reason for hiding this comment

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

I'm actually wondering, is there a place where we benefit from using inspect here as opposed to simply String(data.conf[key])? Because as far as I can tell, conf is generally number, string or true/false.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do because we will actually be able to distinguish the data types by using util.inspect. By only converting it to a string, the string 'true' would look the same as the boolean true.

Copy link
Member

@apapirovski apapirovski Feb 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

That's fair.

Personally, I'm not sure I actually love that part of the functionality but obviously that's not something that needs to be addressed in this PR. Since it's generating CSV, having true and "true" be equivalent is almost preferable to me.

One other thing... util.inspect will do the following:

util.inspect("true");
// '\'true\''
JSON.stringify("true");
// '"true"'

The former doesn't seem correct for the purposes of CSV.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW one of the problems with single quotes is there seems to be no accepted way of handling them. Some parsers seem to treat them as equivalent to double quotes (meaning they need to be escaped) and others treat them as just a character.

Copy link
Member

Choose a reason for hiding this comment

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

@apapirovski I wrote this with the RFC 4180 specs in mind. If a parser does something else (and many do), then that is just tough luck. Creating CSV files that work in all parsers is a very big challenge. We just need to make sure it works with read.csv in R.

benchmark/compare.js Outdated
}
conf = conf.slice(1);
// Escape quotes (") for correct csv formatting
conf = conf.replace(/"/g, '""');
Copy link
Member

Choose a reason for hiding this comment

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

Seconding this question.

apapirovski
apapirovski approved these changes Feb 6, 2018
View reviewed changes
Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

Ignoring CSV silliness, this seems good to me.

AndreasMadsen
AndreasMadsen approved these changes Feb 6, 2018
View reviewed changes
Copy link
Member

@AndreasMadsen AndreasMadsen left a comment

Choose a reason for hiding this comment

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

LGTM, you should run the benchmark CI job on something fast, just to make sure it works.

@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 7, 2018

CI https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/116/

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 7, 2018
@apapirovski
Copy link
Member

Landed in 809af1f

@apapirovski apapirovski closed this Feb 8, 2018
apapirovski pushed a commit that referenced this pull request Feb 8, 2018
@BridgeAR @apapirovski
benchmark: improve compare output
809af1f 
The current output uses JSON.stringify to escape the config values.
This switches to util.inspect to have a better readable output.

PR-URL: #18597
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@BridgeAR @MylesBorins
benchmark: improve compare output
10e861c 
The current output uses JSON.stringify to escape the config values.
This switches to util.inspect to have a better readable output.

PR-URL: #18597
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@BridgeAR @MylesBorins
benchmark: improve compare output
e7f8b4b 
The current output uses JSON.stringify to escape the config values.
This switches to util.inspect to have a better readable output.

PR-URL: #18597
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@BridgeAR @MylesBorins
benchmark: improve compare output
1072f02 
The current output uses JSON.stringify to escape the config values.
This switches to util.inspect to have a better readable output.

PR-URL: #18597
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@BridgeAR @MylesBorins
benchmark: improve compare output
4d78eb8 
The current output uses JSON.stringify to escape the config values.
This switches to util.inspect to have a better readable output.

PR-URL: #18597
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
gibfahn pushed a commit that referenced this pull request Apr 13, 2018
@BridgeAR @gibfahn
benchmark: improve compare output
86e3c89 
The current output uses JSON.stringify to escape the config values.
This switches to util.inspect to have a better readable output.

PR-URL: #18597
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@BridgeAR @MayaLekova
benchmark: improve compare output
11bc671 
The current output uses JSON.stringify to escape the config values.
This switches to util.inspect to have a better readable output.

PR-URL: nodejs#18597
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@BridgeAR BridgeAR deleted the minor-benchmark-improvement branch April 1, 2019 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@AndreasMadsen AndreasMadsen AndreasMadsen approved these changes

@jasnell jasnell jasnell approved these changes

@apapirovski apapirovski apapirovski approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. benchmark Issues and PRs related to the benchmark subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@BridgeAR @apapirovski @jasnell @AndreasMadsen @lpinca @gibfahn @nodejs-github-bot