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: convert var to es6 const, let #12886

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
@sebasmurphy
Contributor

sebasmurphy commented May 7, 2017

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), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

benchmark

benchmark: convert var to es6 const, let
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.
@refack

refack requested changes May 7, 2017 edited

1. Revert the lets
2. show there is no performance regression - benchmark guide

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack May 7, 2017

Member

@sebasmurphy we forgot to say thank you.
Well, thank you! 🥇

Member

refack commented May 7, 2017

@sebasmurphy we forgot to say thank you.
Well, thank you! 🥇

@sebasmurphy

This comment has been minimized.

Show comment
Hide comment
@sebasmurphy

sebasmurphy May 8, 2017

Contributor

Thanks @refack. Still need to do the performance comparison.

Contributor

sebasmurphy commented May 8, 2017

Thanks @refack. Still need to do the performance comparison.

@lpinca

lpinca approved these changes May 8, 2017

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell May 9, 2017

Member

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)

Member

jasnell commented May 9, 2017

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)

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Jun 16, 2017

Member

Ref: #13729

Member

refack commented Jun 16, 2017

Ref: #13729

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Aug 17, 2017

Member

Is this stalled or blocked on @refack's re-review?

Member

bnoordhuis commented Aug 17, 2017

Is this stalled or blocked on @refack's re-review?

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Aug 17, 2017

Member

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/

Member

refack commented Aug 17, 2017

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

refack approved these changes Aug 17, 2017

TF&I perturbed everything anyway, so it's a good opportunity to improve the code.

@tniessen

Change itself LGTM, even though the refactoring of the type array blows up the diff size.

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Aug 17, 2017

Member

Change itself LGTM, even though the refactoring of the type array blows up the diff size.

a trick @Trott tought me: add ?W=1 to the diff url - https://github.com/nodejs/node/pull/12886/files?w=1
Can use this as a bookmarklet: javascript:window.location.search = window.location.search + (window.location.search ? '&' : '?') + 'w=1';

Member

refack commented Aug 17, 2017

Change itself LGTM, even though the refactoring of the type array blows up the diff size.

a trick @Trott tought me: add ?W=1 to the diff url - https://github.com/nodejs/node/pull/12886/files?w=1
Can use this as a bookmarklet: javascript:window.location.search = window.location.search + (window.location.search ? '&' : '?') + 'w=1';

@refack refack removed the V8 Engine label Aug 17, 2017

refack added a commit that referenced this pull request Aug 17, 2017

benchmark: convert var to es6 const
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>
@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Aug 17, 2017

Member

Landed in c49dcb3
@sebasmurphy thank you for first contribution and welcome.
Congratulations on GitHub promoting from:
image
to
image
🍾 🍰
If you would like to submit the conversion of the rest of the vars to lets, now that TF&I landed I think it will be most welcome.

Member

refack commented Aug 17, 2017

Landed in c49dcb3
@sebasmurphy thank you for first contribution and welcome.
Congratulations on GitHub promoting from:
image
to
image
🍾 🍰
If you would like to submit the conversion of the rest of the vars to lets, now that TF&I landed I think it will be most welcome.

@refack refack added the lts-agenda label Aug 17, 2017

@jasnell jasnell closed this Aug 18, 2017

MylesBorins added a commit that referenced this pull request Sep 10, 2017

benchmark: convert var to es6 const
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>

@MylesBorins MylesBorins referenced this pull request Sep 10, 2017

Merged

v8.5.0 proposal #15308

MylesBorins added a commit that referenced this pull request Sep 12, 2017

benchmark: convert var to es6 const
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>

MylesBorins added a commit that referenced this pull request Sep 20, 2017

benchmark: convert var to es6 const
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>

@MylesBorins MylesBorins removed the lts-agenda label Sep 20, 2017

@MylesBorins MylesBorins referenced this pull request Sep 20, 2017

Merged

v6.11.4 proposal #15506

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment