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

benchmark, http: refactor for code consistency by RamirezAlex · Pull Request #28791 · nodejs/node · GitHub

/ node Public

benchmark, http: refactor for code consistency - #28791

Closed
RamirezAlex wants to merge 1 commit into
nodejs:masterfrom
RamirezAlex:benchmark-http-refactor
Closed

benchmark, http: refactor for code consistency#28791
RamirezAlex wants to merge 1 commit into
nodejs:masterfrom
RamirezAlex:benchmark-http-refactor

Conversation

RamirezAlex commented Jul 21, 2019
edited
Loading

Copy link
Copy Markdown
Contributor

In benchmark http directory this changes for loops using var to let
when it applies for consistency and it always add an empty line
after 'use strict'.

If this is OK, I will do it for the other directories in benchmark with single commits so it's easy to review a few files per commit.

  • make -j4 test (UNIX)
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. http Issues or PRs related to the http subsystem. labels Jul 21, 2019

Trott commented Jul 21, 2019

Copy link
Copy Markdown
Member

Older versions of V8 (and thus older versions of Node.js) had significant performance degradation when let was used in a loop. Since we sometimes run benchmark code with old versions of Node.js to see how the performance has changed, using var may be preferred here. We want to benchmark the code in the loop, not the loop itself, so minimizing the performance difference of the loop itself is desirable.

Copy link
Copy Markdown
Contributor Author

Thank you @Trott for explaining this to me.

I am OK closing it. However, could you please let me know if the other around works? Meaning to have always var in the for loops, I mean is there some sort of criteria on when to use let and when to use var in the loops of the benchmarks. I am asking because it looked a little random for me. :)

In the other hand, what about the empty line after 'use strict' is there some sort of consensus there? Thank you.

Trott commented Jul 21, 2019

Copy link
Copy Markdown
Member

I am OK closing it.

Let's leave it open to see what other Collaborators think. For example, there may be consensus that the benchmark common module is sufficiently incompatible with old versions of Node.js that the concern I raised doesn't really matter anymore.

However, could you please let me know if the other around works? Meaning to have always var in the for loops, I mean is there some sort of criteria on when to use let and when to use var in the loops of the benchmarks. I am asking because it looked a little random for me. :)

I think let in loops is OK everywhere now except maybe in benchmarks as I noted above. It's possible that there is some particularly performance-sensitive code in lib where var is preferable for some microscopic performance benefit, but I don't think so.

In the other hand, what about the empty line after 'use strict' is there some sort of consensus there?

I don't have an opinion on that, other than that if we're going to require an empty line after 'use strict', I'd like to see that enforced by an ESLint rule. To me personally, it doesn't really matter if some files have a blank line after 'use strict' and others do not. But if that specific consistency is important to others, I'm not going to stop it. But I would definitely like to see it enforced by eslint (or maybe even prettier).

addaleax commented Jul 21, 2019
edited
Loading

Copy link
Copy Markdown
Member

I’m okay with the var → let conversion, because that means we’re closer to benchmarking idiomatic JS.

We’re pretty evenly split on the blank line issue in our current code, with 42 % percent of JS files in lib/, benchmark/ and test/ using a blank line. Personally, I don’t see why we would add a blank line to files.

RamirezAlex commented Jul 21, 2019
edited
Loading

Copy link
Copy Markdown
Contributor Author

Thank you @Trott and @addaleax for the feedback and clarification.

I feel like the var -> let conversion and blank line after 'use strict' issue should be treated separately. I will remove the extra blank lines I added in this PR and update the commit message so it is only related to the var -> let and I will open later an issue with the blank line thing including the eslint rule to keep the discussion there.

I don't really much care if it is either adding or removing the blank line, but I think there should be some consistency.

Does it sound like a plan?

Copy link
Copy Markdown
Member

@RamirezAlex I think separating the two concerns into different PRs sounds fine 👍

In benchmark http directory this changes for loops using var to let
when it applies for consistency

Trott left a comment

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

Other benchmarks use for (let so might as well be consistent. Thanks.

$ grep -l 'for (let' benchmark/*/*.js
benchmark/child_process/child-process-params.js
benchmark/dns/lookup-promises.js
benchmark/es/spread-assign.js
benchmark/es/string-concatenations.js
benchmark/es/string-repeat.js
benchmark/http/_chunky_http_client.js
benchmark/http/create-clientrequest.js
benchmark/http/incoming_headers.js
benchmark/path/parse-posix.js
benchmark/path/parse-win32.js
benchmark/path/relative-posix.js
benchmark/path/relative-win32.js
benchmark/timers/set-immediate-breadth-args.js
benchmark/timers/set-immediate-breadth.js
benchmark/util/priority-queue.js
$

This comment has been minimized.

nodejs-github-bot commented Jul 25, 2019
edited by Trott
Loading

Copy link
Copy Markdown
Collaborator

Trott added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jul 26, 2019
RamirezAlex mentioned this pull request Jul 29, 2019
3 tasks

Trott commented Jul 30, 2019

Copy link
Copy Markdown
Member

Landed in 9140f62

Trott closed this Jul 30, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Jul 30, 2019
In benchmark http directory this changes for loops using var to let
when it applies for consistency

PR-URL: nodejs#28791
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Aug 2, 2019
In benchmark http directory this changes for loops using var to let
when it applies for consistency

PR-URL: #28791
Reviewed-By: Rich Trott <rtrott@gmail.com>
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. benchmark Issues and PRs related to the benchmark subsystem. http Issues or PRs related to the http subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL