FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

benchmark: shorten config name in http benchmark by joyeecheung · Pull Request #18452 · nodejs/node · GitHub

/ node Public

benchmark: shorten config name in http benchmark - #18452

Closed
joyeecheung wants to merge 2 commits into
nodejs:masterfrom
joyeecheung:http-bench-key
ConversationCommits (2)ChecksFiles changed
Closed

benchmark: shorten config name in http benchmark#18452
joyeecheung wants to merge 2 commits into
nodejs:masterfrom
joyeecheung:http-bench-key

Conversation

joyeecheung commented Jan 30, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Before the output of compare.R run on http benchmarks contains a line like this

 http/check_invalid_header_char.js n=1000000 key="Here is a value that is really a folded header value\\r\\n  this should be      supported, but it is not currently"       ±4.32%  ±5.75%  ±7.49%

which force every line to align with it so the results are hard to read (both from a console and from the benchmark CI) due to wrapping. (For example, see the end of https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/107/console)

After this patch the line would look like this

 http/check_invalid_header_char.js n=1000000 key="LONG_AND_INVALID"       ±4.32%  ±5.75%  ±7.49%

and the results won't be as wide and will not usually be wrapped anymore.

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

benchmark: shorten config name in http benchmark
953c941 
Shorten the config name in check_invalid_header_char so it would
not result in long lines that make the benchmark result hard to read.
nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. http Issues or PRs related to the http subsystem. labels Jan 30, 2018
lpinca approved these changes Jan 30, 2018
View reviewed changes
Comment thread benchmark/http/check_invalid_header_char.js Outdated
});

function main({ n, key }) {
if (key == 'LONG_AND_INVALID') {

lpinca Jan 30, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit: ===

joyeecheung commented Feb 1, 2018

Copy link
Copy Markdown
Member Author

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

joyeecheung added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 1, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 1, 2018
benchmark: shorten config name in http benchmark
d3841ec 
Shorten the config name in check_invalid_header_char so it would
not result in long lines that make the benchmark result hard to read.

PR-URL: nodejs#18452
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

BridgeAR commented Feb 1, 2018

Copy link
Copy Markdown
Member

Landed in d3841ec

I fixed the linting issue while landing.

BridgeAR closed this Feb 1, 2018
addaleax removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 4, 2018
MylesBorins pushed a commit that referenced this pull request Feb 20, 2018
benchmark: shorten config name in http benchmark
20f33e4 
Shorten the config name in check_invalid_header_char so it would
not result in long lines that make the benchmark result hard to read.

PR-URL: #18452
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
benchmark: shorten config name in http benchmark
cea50ff 
Shorten the config name in check_invalid_header_char so it would
not result in long lines that make the benchmark result hard to read.

PR-URL: #18452
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
benchmark: shorten config name in http benchmark
9ae513a 
Shorten the config name in check_invalid_header_char so it would
not result in long lines that make the benchmark result hard to read.

PR-URL: #18452
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins mentioned this pull request Feb 21, 2018
Merged

MylesBorins commented Mar 20, 2018

Copy link
Copy Markdown
Contributor

Should this be backported to v8.x-staging or v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
benchmark: shorten config name in http benchmark
df45bb0 
Shorten the config name in check_invalid_header_char so it would
not result in long lines that make the benchmark result hard to read.

PR-URL: nodejs#18452
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
lpinca lpinca approved these changes
BridgeAR BridgeAR approved these changes
+1 more reviewer
starkwang starkwang approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

benchmark Issues and PRs related to the benchmark subsystem. http Issues or PRs related to the http subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL