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, http: refactor for code consistency #28791

Closed

Conversation

@RamirezAlex
Copy link
Member

commented Jul 21, 2019

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
@Trott

This comment has been minimized.

Copy link
Member

commented Jul 21, 2019

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.

@RamirezAlex

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2019

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

This comment has been minimized.

Copy link
Member

commented Jul 21, 2019

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

This comment has been minimized.

Copy link
Member

commented Jul 21, 2019

I’m okay with the varlet 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

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2019

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?

@addaleax

This comment has been minimized.

Copy link
Member

commented Jul 21, 2019

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

@RamirezAlex RamirezAlex force-pushed the RamirezAlex:benchmark-http-refactor branch from f0e4b75 to 359d611 Jul 21, 2019

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

@RamirezAlex RamirezAlex force-pushed the RamirezAlex:benchmark-http-refactor branch from 359d611 to db9f71b Jul 21, 2019

@Trott
Trott approved these changes Jul 24, 2019
Copy link
Member

left a comment

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
$
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment has been minimized.

Copy link

commented Jul 25, 2019

@Trott Trott added the author ready label Jul 26, 2019

@RamirezAlex RamirezAlex referenced this pull request Jul 29, 2019
3 of 3 tasks complete
@Trott

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Landed in 9140f62

@Trott Trott closed this Jul 30, 2019

Trott added a commit to Trott/io.js that referenced this pull request Jul 30, 2019
benchmark, http: refactor for code consistency
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 added a commit that referenced this pull request Aug 2, 2019
benchmark, http: refactor for code consistency
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>
@BridgeAR BridgeAR referenced this pull request Aug 6, 2019
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
benchmark, http: refactor for code consistency
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>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
benchmark, http: refactor for code consistency
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.