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 swap var for let refactor in for loops #28958

Closed
wants to merge 6 commits into from

Conversation

RamirezAlex
Copy link
Contributor

@RamirezAlex RamirezAlex commented Aug 4, 2019

In benchmark directories this changes for loops using var to let
when it applies for consistency. So it finishes what started on #28867

The list of files below remain using var in the nested for loops, rest which is the majority use let now for consistency.

$ grep -l 'for (var' benchmark/*/*.js
benchmark/buffers/buffer-fill.js
benchmark/buffers/buffer-iterate.js
benchmark/buffers/buffer-swap.js
benchmark/cluster/echo.js
benchmark/es/foreach-bench.js
benchmark/events/ee-add-remove.js
benchmark/fs/read-stream-throughput.js
benchmark/misc/freelist.js
benchmark/module/module-loader.js
benchmark/streams/readable-bigread.js
benchmark/streams/readable-bigunevenread.js
benchmark/streams/readable-boundaryread.js
benchmark/streams/readable-readall.js
benchmark/streams/readable-unevenread.js
benchmark/string_decoder/string-decoder.js
benchmark/util/normalize-encoding.js
benchmark/worker/echo.js
  • make -j4 test (UNIX)
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. benchmark Issues and PRs related to the benchmark subsystem. cluster Issues and PRs related to the cluster subsystem. crypto Issues and PRs related to the crypto subsystem. dgram Issues and PRs related to the dgram subsystem / UDP. domain Issues and PRs related to the domain subsystem. events Issues and PRs related to the events subsystem / EventEmitter. fs Issues and PRs related to the fs subsystem / file system. v8 engine Issues and PRs related to the V8 dependency. labels Aug 4, 2019
@RamirezAlex RamirezAlex changed the title Benchmark swap var for let refactor in for loops benchmark swap var for let refactor in for loops Aug 4, 2019
@Trott
Copy link
Member

Trott commented Aug 7, 2019

@Trott
Copy link
Member

Trott commented Aug 7, 2019

Benchmark CI: https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/8415/ (queued, will 404 until a worker becomes available)

@Trott
Copy link
Member

Trott commented Aug 25, 2019

@mscdex Are you 👍, 👎 , or ❓ on these changes in the benchmark files?

@RamirezAlex
Copy link
Contributor Author

Hi @Trott, I have resolved some conflicts that we recently had in this PR after more benchmarks code were introduced.
Did the Benchkmark CI queue you run lasts time went OK? Should we run those benckmarks again?
Thank you.

@BridgeAR
Copy link
Member

This needs a rebase. @RamirezAlex sorry that it took so long for someone to look at this again! Some times it's difficult to keep an overview over everything.

@RamirezAlex RamirezAlex force-pushed the benchmark-refactor branch 2 times, most recently from 70c8fc1 to 54b4c84 Compare February 2, 2020 08:39
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Feb 3, 2020

In benchmark timers directory this changes for loops using var to let
when it applies for consistency
In benchmark path directory this changes for loops
using var to let when it applies for consistency
In benchmark os directory this changes for loops
using var to let when it applies for consistency
In benchmark querystring directory this changes for loops
using var to let when it applies for consistency
In benchmark vm directory this changes for loops
using var to let when it applies for consistency
In benchmark directory this changes for loops
using var to let when it applies for consistency
@Trott
Copy link
Member

Trott commented Feb 3, 2020

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

addaleax pushed a commit that referenced this pull request Feb 13, 2020
In benchmark directory this changes for loops
using var to let when it applies for consistency

PR-URL: #28958
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@addaleax
Copy link
Member

Landed in d0ed431 🎉

@addaleax addaleax closed this Feb 13, 2020
codebytere pushed a commit that referenced this pull request Feb 17, 2020
In benchmark directory this changes for loops
using var to let when it applies for consistency

PR-URL: #28958
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@codebytere codebytere mentioned this pull request Feb 17, 2020
codebytere pushed a commit that referenced this pull request Mar 15, 2020
In benchmark directory this changes for loops
using var to let when it applies for consistency

PR-URL: #28958
Reviewed-By: Anna Henningsen <anna@addaleax.net>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
In benchmark directory this changes for loops
using var to let when it applies for consistency

PR-URL: #28958
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@codebytere codebytere mentioned this pull request Mar 17, 2020
codebytere pushed a commit that referenced this pull request Mar 30, 2020
In benchmark directory this changes for loops
using var to let when it applies for consistency

PR-URL: #28958
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. benchmark Issues and PRs related to the benchmark subsystem. cluster Issues and PRs related to the cluster subsystem. crypto Issues and PRs related to the crypto subsystem. dgram Issues and PRs related to the dgram subsystem / UDP. domain Issues and PRs related to the domain subsystem. events Issues and PRs related to the events subsystem / EventEmitter. fs Issues and PRs related to the fs subsystem / file system. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants