Skip to content

timers: remove no longer relevant comment#14314

Closed
timcosta wants to merge 2 commits intonodejs:masterfrom
timcosta:resolve_14308
Closed

timers: remove no longer relevant comment#14314
timcosta wants to merge 2 commits intonodejs:masterfrom
timcosta:resolve_14308

Conversation

@timcosta
Copy link
Contributor

@timcosta timcosta commented Jul 16, 2017

This PR resolves #14308 by removing a comment that is no longer relevant.

Checklist
Affected core subsystem(s)

None. File modified is timers, but no code was changed.

@nodejs-github-bot nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Jul 16, 2017
@mscdex
Copy link
Contributor

mscdex commented Jul 17, 2017

I don't think the comment is needed at all for v7.x+ since V8 in v7.x allows optimization of functions containing try-catch/finally, they just can't be inlined. With TurboFan it's probably even less of an issue (I have not checked inlineability with try-catch/finally there).

@gibfahn
Copy link
Member

gibfahn commented Jul 17, 2017

@mscdex in that case I guess the call should be moved inside the try/finally and the comment removed?

@mscdex
Copy link
Contributor

mscdex commented Jul 17, 2017 •
edited
Loading

@gibfahn I'm not sure manually inlining the helper function really matters, although I haven't benchmarked the difference with TurboFan.

Fishrock123
Fishrock123 previously requested changes Jul 17, 2017
View reviewed changes
Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

Refer to the discussion in #14308

This should probably revert the optimization altogether once we have TurboFan (do we have it by default in master? not sure lol).

The change is just extra noise otherwise. :/

@addaleax
Copy link
Member

addaleax commented Jul 17, 2017

once we have TurboFan (do we have it by default in master? not sure lol).

Yes. :)

@benjamingr
Copy link
Member

benjamingr commented Sep 1, 2017

Ping @Fishrock123

@BridgeAR BridgeAR mentioned this pull request Sep 5, 2017
Closed
1 task
@BridgeAR
Copy link
Member

BridgeAR commented Sep 8, 2017

I think it would be fine to land this even if our real goal is to change the code again. This is a task marked as good first contribution and it would be good to land this as is.

@BridgeAR BridgeAR self-assigned this Sep 9, 2017
@BridgeAR
Copy link
Member

BridgeAR commented Sep 9, 2017

@Fishrock123 if you still have concerns, please respond here. Otherwise I am going to land this on Monday.

Fishrock123
Fishrock123 approved these changes Sep 9, 2017
View reviewed changes
Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

sure

@BridgeAR
Copy link
Member

BridgeAR commented Sep 9, 2017

@Fishrock123 thanks ❤️

@BridgeAR
Copy link
Member

BridgeAR commented Sep 9, 2017

Landed in a2b6872

@BridgeAR BridgeAR closed this Sep 9, 2017
BridgeAR pushed a commit that referenced this pull request Sep 9, 2017
@timcosta @BridgeAR
timers: fix outdated comment
a2b6872 
PR-URL: #14314
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
@timcosta @MylesBorins
timers: fix outdated comment
dfdb619 
PR-URL: #14314
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Sep 11, 2017
@timcosta @MylesBorins
timers: fix outdated comment
82017c1 
PR-URL: #14314
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
@timcosta @MylesBorins
timers: fix outdated comment
dae86e4 
PR-URL: #14314
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
@timcosta @addaleax
timers: fix outdated comment
9a1b34c 
PR-URL: nodejs#14314
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
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

@addaleax addaleax addaleax approved these changes

@benjamingr benjamingr benjamingr approved these changes

@tniessen tniessen tniessen approved these changes

+2 more reviewers

@Fishrock123 Fishrock123 Fishrock123 approved these changes

@gibfahn gibfahn gibfahn approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@BridgeAR BridgeAR

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.

Outdated comment in timers

10 participants

@timcosta @mscdex @gibfahn @addaleax @benjamingr @BridgeAR @jasnell @Fishrock123 @tniessen @nodejs-github-bot