benchmark: convert var to es6 const, let#12886
benchmark: convert var to es6 const, let#12886sebasmurphy wants to merge 3 commits intonodejs:masterfrom sebasmurphy:var-to-const
Conversation
Converted var variable to es6 const and let to maintain consistency with other benchmark files. Also clean up the types array to make the files more succinct.
nodejs-github-bot
added
benchmark
benchmark/arrays/var-int.js
Outdated
| var arr = new clazz(n * 1e6); | ||
| for (var i = 0; i < 10; ++i) { | ||
| let arr = new clazz(n * 1e6); | ||
| for (let i = 0; i < 10; ++i) { |
There was a problem hiding this comment.
We don't use let in the benchmarks, and let causes a noticeable performance deficit with current V8.
There was a problem hiding this comment.
@sebasmurphy wait with the lets a month or two until we activate TurboFan and Ignition nodejs/CTC#99
There was a problem hiding this comment.
1. Revert the lets
2. show there is no performance regression - benchmark guide
benchmark/arrays/zero-int.js
Outdated
| bench.start(); | ||
| var arr = new clazz(n * 1e6); | ||
| for (var i = 0; i < 10; ++i) { | ||
| for (let i = 0; i < 10; ++i) { |
|
@sebasmurphy we forgot to say thank you. |
|
Thanks @refack. Still need to do the performance comparison. |
|
I have no issues with the code itself, but just want to provide the context. Historically, we have avoided making such changes in the benchmarks in order to allow us to keep running the benchmarks against older versions of Node.js that do not have these constructs (think, 0.10 and 0.12). That said, now that all of those older versions are no longer supported, it should be perfectly fine to re-baseline the benchmarks on whatever is supported by Node.js 4.x. (or, moving forward, the oldest release line in LTS at any given point in time) |
|
Ref: #13729 |
|
Is this stalled or blocked on @refack's re-review? |
|
It was mostly blocked till TF&I landed (#13729). So in my POV this could move forward. Pre-land CI: https://ci.nodejs.org/job/node-test-pull-request/9713/ |
refack
left a comment
refack
left a comment
There was a problem hiding this comment.
TF&I perturbed everything anyway, so it's a good opportunity to improve the code.
tniessen
left a comment
tniessen
left a comment
There was a problem hiding this comment.
Change itself LGTM, even though the refactoring of the type array blows up the diff size.
a trick @Trott tought me: add |
Converted var variable to es6 const to maintain consistency with other benchmark files. Also clean up the types array to make the files more succinct. PR-URL: #12886 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
|
Landed in c49dcb3 |
Converted var variable to es6 const to maintain consistency with other benchmark files. Also clean up the types array to make the files more succinct. PR-URL: #12886 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Converted var variable to es6 const to maintain consistency with other benchmark files. Also clean up the types array to make the files more succinct. PR-URL: #12886 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Converted var variable to es6 const to maintain consistency with other benchmark files. Also clean up the types array to make the files more succinct. PR-URL: #12886 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
11 participants
Converted var variable to es6 const and let to maintain
consistency with other benchmark files. Also clean up
the types array to make the files more succinct.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
benchmark