Skip to content

internal: changed var to const in linkedlist#8609

Closed
AdriVanHoudt wants to merge 1 commit into
nodejs:masterfrom
AdriVanHoudt:const_linkedlist
Closed

internal: changed var to const in linkedlist#8609
AdriVanHoudt wants to merge 1 commit into
nodejs:masterfrom
AdriVanHoudt:const_linkedlist

Conversation

@AdriVanHoudt

@AdriVanHoudt AdriVanHoudt commented Sep 17, 2016 •
edited by Fishrock123
Loading

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

internal

Description of change

Changed var to const.
Part of nodejs/code-and-learn#56

Refs: #8410

@nodejs-github-bot nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Sep 17, 2016
@targos

targos commented Sep 17, 2016

Copy link
Copy Markdown
Member

LGTM

cjihrig
cjihrig approved these changes Sep 17, 2016
View reviewed changes

@cjihrig cjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@mscdex

mscdex commented Sep 17, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

This may sound a bit crazy, but we may want to benchmark this change. I noticed not too long ago that var => const was actually causing slowdowns and timers are definitely a hot path. I do not know if this has changed with V8 5.4 or not though.

@AdriVanHoudt

AdriVanHoudt commented Sep 17, 2016

Copy link
Copy Markdown
Contributor Author

@mscdex if there is anything I can help with let me know. I do not know how to run benchmarks atm so yeah :P

@Fishrock123 Fishrock123 mentioned this pull request Sep 17, 2016
Closed
@mscdex

mscdex commented Sep 19, 2016

Copy link
Copy Markdown
Contributor

Ok as far as I can tell, this particular change does not seem to have any measurable negative performance impact, so it should be fine to land.

On another positive note, I think I managed to improve setImmediate() performance while I was digging in the code.

jasnell
jasnell approved these changes Sep 20, 2016
View reviewed changes

@jasnell jasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@jasnell

jasnell commented Sep 20, 2016

Copy link
Copy Markdown
Member

@AdriVanHoudt ... now that you have a few of these type of PRs in, one thing that would likely be helpful would be to batch multiple commits either into a single PR or even single commit. Doing so makes review a bit easier.

@mscdex

mscdex commented Sep 20, 2016

Copy link
Copy Markdown
Contributor

LGTM.

@AdriVanHoudt

AdriVanHoudt commented Sep 20, 2016

Copy link
Copy Markdown
Contributor Author

@jasnell yeah during the summit the task got divided pretty much by file that's why. Also some changes do entail a lot more (like perf stuff) then I thought so the splitting kinda helped there. I will definitely do more in 1 commit next time! (or do you want me to merge all these together?)

@jasnell

jasnell commented Sep 20, 2016

Copy link
Copy Markdown
Member

nah, go ahead and leave these as is. Just wanted to make the suggestion for the future! :-)

@addaleax

addaleax commented Sep 20, 2016

Copy link
Copy Markdown
Member

tbh, I’d still prefer 1 commit per file, if only because that may make backporting easier.

@jasnell

jasnell commented Sep 20, 2016

Copy link
Copy Markdown
Member

ok, yeah, makes sense. At the very least tho, multiple similar commits per PR.

@jasnell

jasnell commented Sep 22, 2016

Copy link
Copy Markdown
Member

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

@imyller

imyller commented Oct 3, 2016

Copy link
Copy Markdown
Member

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

@AdriVanHoudt

AdriVanHoudt commented Oct 4, 2016

Copy link
Copy Markdown
Contributor Author

Jenkins is kinda hard to navigate so not sure what is failing :S

@addaleax

addaleax commented Oct 5, 2016

Copy link
Copy Markdown
Member

… Yeah.

New CI, given that #8924 has fixed some of the failures: https://ci.nodejs.org/job/node-test-commit/5455/

@jasnell jasnell self-assigned this Oct 6, 2016
jasnell pushed a commit that referenced this pull request Oct 6, 2016
@AdriVanHoudt @jasnell
lib: changed var to const in linkedlist
b2534f1 
PR-URL: #8609
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@jasnell

jasnell commented Oct 6, 2016

Copy link
Copy Markdown
Member

Landed in b2534f1

@jasnell jasnell closed this Oct 6, 2016
@AdriVanHoudt AdriVanHoudt deleted the const_linkedlist branch October 7, 2016 08:03
jasnell pushed a commit that referenced this pull request Oct 10, 2016
@AdriVanHoudt @jasnell
lib: changed var to const in linkedlist
9578038 
PR-URL: #8609
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
@AdriVanHoudt @Fishrock123
lib: changed var to const in linkedlist
313a45d 
PR-URL: #8609
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 22, 2016
@AdriVanHoudt
lib: changed var to const in linkedlist
3b448a7 
PR-URL: #8609
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorins MylesBorins mentioned this pull request Nov 22, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees

@jasnell jasnell

Labels

timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@AdriVanHoudt @targos @mscdex @jasnell @addaleax @imyller @cjihrig @MylesBorins @nodejs-github-bot