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.

Sign up for GitHub

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

Jump to bottom

benchmark: var to const #13757

Closed
wants to merge 1 commit into from
Closed

benchmark: var to const #13757

wants to merge 1 commit into from

Conversation

BridgeAR
Copy link
Member

As discussed in #13732: this will change all var to const if applicable.
The main question is if this is wanted due to the churn.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

benchmark

@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. buffer Issues and PRs related to the buffer subsystem. child_process Issues and PRs related to the child_process 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. v8 engine Issues and PRs related to the V8 dependency. labels Jun 18, 2017
@mscdex mscdex removed buffer Issues and PRs related to the buffer subsystem. child_process Issues and PRs related to the child_process 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. v8 engine Issues and PRs related to the V8 dependency. labels Jun 18, 2017
tniessen
tniessen reviewed Jun 18, 2017
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion on this if someone can confirm that there is no performance degression.

benchmark/process/next-tick-depth.js Outdated
@@ -1,6 +1,7 @@
'use strict';
var common = require('../common.js');
var bench = common.createBenchmark(main, {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason why you introduced a LF here but not in other files?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It stumbled over it while I cherry-picked this part from the other PR due to a conflict in this file. I can keep it as it is if you want me to undo it.

@tniessen tniessen self-assigned this Jun 18, 2017
@tniessen
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/8724/

@jasnell
Copy link
Member

jasnell commented Jun 19, 2017

Historically, we have held off updating the code in benchmarks so that we can more realistically compare performance with older Node.js versions. We should be careful making these kinds of changes. @mscdex ... thoughts?

@mscdex
Copy link
Contributor

mscdex commented Jun 19, 2017

I don't have any opinion, we currently have mixed const usage throughout all benchmarks. *shrug*

@BridgeAR
Copy link
Member Author

Is there any conclusion about how to further progress? I guess as the benchmarks already contain const, this is a safe thing?

Note: the benchmarks change fast and I did this change by having a own eslint fix rule, so I would rebase as soon as there's a conclusion about how to go on.

@BridgeAR
Copy link
Member Author

Just as info - I am going to update this as soon as v8 6.1 lands. I think we are safe to land this because we already use const in the benchmarks and it will only only deopt in rare cases in older v8 versions.

@jasnell
Copy link
Member

jasnell commented Aug 25, 2017

I'm marking it blocked for now then. Please remove the label when you're ready to update!

@jasnell jasnell added blocked PRs that are blocked by other issues or PRs. wip Issues and PRs that are still a work in progress. labels Aug 25, 2017
@BridgeAR BridgeAR removed the blocked PRs that are blocked by other issues or PRs. label Sep 13, 2017
@BridgeAR BridgeAR removed the wip Issues and PRs that are still a work in progress. label Sep 14, 2017
@BridgeAR
Copy link
Member Author

Rebased. This should now be possible to land since we got v8 6.1 on master.

mcollina
mcollina approved these changes Sep 14, 2017
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mcollina
Copy link
Member

@bmeurer what do you think?

@BridgeAR
Copy link
Member Author

I would like to land this soon because the code base changes fast and I have to rebase often otherwise.

bmeurer
bmeurer approved these changes Sep 18, 2017
View reviewed changes
Copy link
Member

@bmeurer bmeurer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup!

@BridgeAR
Copy link
Member Author

Thanks a lot! Landed in e167ab7

@BridgeAR BridgeAR closed this Sep 20, 2017
BridgeAR added a commit that referenced this pull request Sep 20, 2017
@BridgeAR
benchmark: var to const
e167ab7 
PR-URL: #13757
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@jasnell
Copy link
Member

jasnell commented Sep 20, 2017

This requires #14881 backport to land in v8.x before it can be cherry-picked

@BridgeAR BridgeAR mentioned this pull request Sep 20, 2017
Closed
4 tasks
BridgeAR added a commit to BridgeAR/node that referenced this pull request Sep 21, 2017
@BridgeAR
benchmark: var to const
eb62d55 
PR-URL: nodejs#13757
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
jasnell pushed a commit that referenced this pull request Sep 21, 2017
@BridgeAR @jasnell
benchmark: var to const
535f8d5 
PR-URL: #13757
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
@BridgeAR
benchmark: var to const
7c81e73 
PR-URL: nodejs/node#13757
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
@BridgeAR
benchmark: var to const
a544cd5 
PR-URL: nodejs/node#13757
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 17, 2017
Closed
@MylesBorins
Copy link
Contributor

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

This is likely to make benchmarks on 6.x slower though. TBQH I'm not sure this should have landed prior to 6.x going into maintenance mode as it will make future backports harder. That being said benchmarks have diverged quite a bit so it might be a moot point.

Glad to see this was part of the conversation prior to landing.

@BridgeAR
Copy link
Member Author

After looking at this again I do not really see the necessity for a backport here. If we add a new benchmark, backporting will be fine all the time. Changing benchmarks happens pretty rarely otherwise. If a backport is really necessary, I am of course fine to do so. It is not really much work as I just use a individual eslint rule for it. I am not sure anymore how bad const was in v.6 but I am pretty sure const was fine in most cases. Especially when looking at the changed variables.

@MylesBorins your call if I should open a backport or not.

@BridgeAR BridgeAR deleted the var-to-const-benchmark branch April 1, 2019 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

@tniessen tniessen tniessen approved these changes

@lpinca lpinca lpinca approved these changes

@bmeurer bmeurer bmeurer approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

@mscdex mscdex Awaiting requested review from mscdex

Assignees

@tniessen tniessen

Labels
benchmark Issues and PRs related to the benchmark subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@BridgeAR @tniessen @jasnell @mscdex @mcollina @MylesBorins @bmeurer @lpinca @gibfahn @nodejs-github-bot