FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

timers: add an internal [@@refresh()] function by Fishrock123 · Pull Request #18065 · nodejs/node · GitHub

/ node Public

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

Closed
Fishrock123 wants to merge 2 commits into
nodejs:masterfrom
Fishrock123:timers-internal-refresh
ConversationCommits (2)ChecksFiles changed
Closed

timers: add an internal [@@refresh()] function#18065
Fishrock123 wants to merge 2 commits into
nodejs:masterfrom
Fishrock123:timers-internal-refresh

Conversation

Fishrock123 commented Jan 9, 2018

Copy link
Copy Markdown
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 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 added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 9, 2018
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 reviewed Jan 9, 2018
View reviewed changes
Comment thread lib/internal/http2/core.js Outdated

jasnell Jan 9, 2018

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

what is the true argument for?

Fishrock123 Jan 9, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

whoops, was from a previous API version

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

Fishrock123 commented Jan 18, 2018

Copy link
Copy Markdown
Contributor Author

@apapirovski @zhangzifa Any thoughts?

apapirovski approved these changes Jan 19, 2018
View reviewed changes

apapirovski 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 force-pushed the timers-internal-refresh branch from 8c5aa92 to a08570c Compare January 26, 2018 17:24

Fishrock123 commented Jan 26, 2018

Copy link
Copy Markdown
Contributor Author

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

@apapirovski better now?

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>
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 force-pushed the timers-internal-refresh branch from a08570c to acb5ad3 Compare January 26, 2018 21:45
Fishrock123 added a commit that referenced this pull request Jan 26, 2018
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
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 commented Jan 26, 2018

Copy link
Copy Markdown
Contributor Author

Thanks, landed in a5a8118...bb5575a

Fishrock123 deleted the timers-internal-refresh branch January 26, 2018 21:49

MylesBorins commented Feb 20, 2018

Copy link
Copy Markdown
Contributor

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

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
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
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 commented Aug 2, 2018

Copy link
Copy Markdown
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 commented Aug 2, 2018

Copy link
Copy Markdown
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 commented Aug 2, 2018

Copy link
Copy Markdown
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 mentioned this pull request Oct 20, 2018
Closed
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell left review comments
+1 more reviewer
apapirovski apapirovski approved these changes
Reviewers whose approvals may not affect merge requirements

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

Back | FazBrowse Home | New Git URL