Skip to content

Conversation

@joyeecheung
Copy link
Member

@joyeecheungjoyeecheung commented Jan 30, 2018

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

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-botnodejs-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
});

functionmain({ n, key }){
if(key=='LONG_AND_INVALID'){
Copy link
Member

Choose a reason for hiding this comment

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

Nit: ===

@joyeecheung
Copy link
MemberAuthor

@joyeecheungjoyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 1, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 1, 2018
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 <[email protected]> Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
@BridgeAR
Copy link
Member

Landed in d3841ec

I fixed the linting issue while landing.

@BridgeARBridgeAR closed this Feb 1, 2018
@addaleaxaddaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 4, 2018
MylesBorins pushed a commit that referenced this pull request Feb 20, 2018
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 <[email protected]> Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
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 <[email protected]> Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
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 <[email protected]> Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Feb 21, 2018
@MylesBorins
Copy link
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
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 <[email protected]> Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

benchmarkIssues and PRs related to the benchmark subsystem.httpIssues or PRs related to the http subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@joyeecheung@BridgeAR@MylesBorins@jasnell@lpinca@starkwang@addaleax@nodejs-github-bot