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

timers: add an internal [@@refresh()] function #18065

Closed
wants to merge 2 commits into from
Closed

timers: add an internal [@@refresh()] function #18065

wants to merge 2 commits into from

Conversation

Fishrock123
Copy link
Contributor

Hidden via a symbol because I'm unsure exactly what the API should look like in the end.

Removes the need to use _unrefActive() for efficiently refreshing timeouts. It still uses it under the hood but that could be replaced with insert() directly if it were in the same file.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

timers

@Fishrock123 Fishrock123 added timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. http2 Issues or PRs related to the http2 subsystem. labels Jan 9, 2018
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 9, 2018
@Fishrock123 Fishrock123 added net Issues and PRs related to the net subsystem. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 9, 2018
jasnell
jasnell reviewed Jan 9, 2018
View reviewed changes
lib/internal/http2/core.js Outdated
@@ -908,7 +907,7 @@ class Http2Session extends EventEmitter {
[kUpdateTimer]() {
if (this.destroyed)
return;
if (this[kTimeout]) _unrefActive(this[kTimeout]);
if (this[kTimeout]) this[kTimeout][refresh_fn_symbol](true);
Copy link
Member

Choose a reason for hiding this comment

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

what is the true argument for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops, was from a previous API version

@Fishrock123 Fishrock123 force-pushed the timers-internal-refresh branch 4 times, most recently from 11f4c34 to 8c5aa92 Compare January 11, 2018 17:19
@Fishrock123
Copy link
Contributor Author

@apapirovski @zhangzifa Any thoughts?

apapirovski
apapirovski approved these changes Jan 19, 2018
View reviewed changes
Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

I would personally prefer if we went with camel case for the symbols, to match the vast majority of new usage, but the rest of this SGTM. 👍

@Fishrock123
Copy link
Contributor Author

Updated, new CI: https://ci.nodejs.org/job/node-test-pull-request/12755/

@apapirovski better now?

@Fishrock123
timers: reposition getTimers definition internally
587c6a5 
Just cleanup so the file makes more future sense.

PR-URL: nodejs#18065
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@Fishrock123
timers: add internal [@@ refresh()] function
acb5ad3 
Hidden via a symbol because I'm unsure exactly what the API should look
like in the end.

Removes the need to use _unrefActive for efficiently refreshing
timeouts.
It still uses it under the hood but that could be replaced with
insert() directly if it were in the same file.

PR-URL: nodejs#18065
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Fishrock123 added a commit that referenced this pull request Jan 26, 2018
@Fishrock123
timers: reposition getTimers definition internally
54fe0a6 
Just cleanup so the file makes more future sense.

PR-URL: #18065
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Fishrock123 added a commit that referenced this pull request Jan 26, 2018
@Fishrock123
timers: add internal [@@ refresh()] function
bb5575a 
Hidden via a symbol because I'm unsure exactly what the API should look
like in the end.

Removes the need to use _unrefActive for efficiently refreshing
timeouts.
It still uses it under the hood but that could be replaced with
insert() directly if it were in the same file.

PR-URL: #18065
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@Fishrock123
Copy link
Contributor Author

Thanks, landed in a5a8118...bb5575a

@Fishrock123 Fishrock123 deleted the timers-internal-refresh branch January 26, 2018 21:49
@MylesBorins
Copy link
Contributor

This does not land cleanly on v9.x, should we backport?

@apapirovski apapirovski mentioned this pull request Feb 26, 2018
Merged
3 tasks
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@Fishrock123 @MayaLekova
timers: reposition getTimers definition internally
c5b13db 
Just cleanup so the file makes more future sense.

PR-URL: nodejs#18065
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@Fishrock123 @MayaLekova
timers: add internal [@@ refresh()] function
0164140 
Hidden via a symbol because I'm unsure exactly what the API should look
like in the end.

Removes the need to use _unrefActive for efficiently refreshing
timeouts.
It still uses it under the hood but that could be replaced with
insert() directly if it were in the same file.

PR-URL: nodejs#18065
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@codebytere
Copy link
Member

@Fishrock123 should this be backported to v8.x? it doesn't land cleanly as we moved lib/timers.js --> lib/internal/timers.js

@Fishrock123
Copy link
Contributor Author

@codebytere It’d be nice to backport — do you know if it depends on other commits of mine, or only the large timers refactor?

I’ll try to get a backport PR up soon...

@codebytere
Copy link
Member

@Fishrock123 my cursory inspection tells me the two timers commits need to be backported in concert but beyond that i think you're good!

@addaleax addaleax mentioned this pull request Oct 20, 2018
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell left review comments

@apapirovski apapirovski apapirovski approved these changes

Assignees
No one assigned
Labels
http2 Issues or PRs related to the http2 subsystem. net Issues and PRs related to the net subsystem. 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.
6 participants
@Fishrock123 @MylesBorins @codebytere @apapirovski @jasnell @nodejs-github-bot