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.

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

timers: do less work in insert #27345

Closed
wants to merge 3 commits into from

Conversation

apapirovski
Copy link
Member

@apapirovski apapirovski commented Apr 22, 2019

Most of the code in insert is only applicable to scheduling non-timers or re-scheduling timers. We can skip most of it in the case of setTimeout, setInterval & setUnrefTimeout.

Second commit fixes a long-standing bug with refresh in cases where it's triggered after the timer already ran its course. Prior to this the refreshed timer would end up always being unrefed, even when it was previously refed.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Apr 22, 2019
@nodejs-github-bot
Copy link
Collaborator

lib/internal/timers.js Outdated Show resolved Hide resolved
lib/internal/timers.js Outdated Show resolved Hide resolved
@apapirovski
Copy link
Member Author

apapirovski commented Apr 22, 2019

@Fishrock123 I've updated with some significant changes in the second commit. There was a bug with refresh that is now fixed and there's a test case provided. (Found it because I was trying to think through and address your comment re: replacing the schedule call in interval branch.)

@Fishrock123 Fishrock123 self-requested a review April 22, 2019 22:10
@nodejs-github-bot
Copy link
Collaborator

@BridgeAR
Copy link
Member

Benchmark with the latest code: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/356/

Results with any significant change:


                                                             confidence improvement accuracy (*)    (**)   (***)
timers/immediate.js type='clear' n=5000000                          ***     19.14 %       ±2.12%  ±2.80%  ±3.60%
timers/immediate.js type='depth' n=5000000                            *      0.83 %       ±0.73%  ±0.96%  ±1.23%
timers/set-immediate-depth-args.js n=5000000                          *     -0.92 %       ±0.78%  ±1.03%  ±1.32%
timers/timers-cancel-unpooled.js direction='end' n=1000000            *     15.14 %      ±12.87% ±16.98% ±21.81%
timers/timers-depth.js n=1000                                         *      0.06 %       ±0.05%  ±0.07%  ±0.09%
timers/timers-insert-unpooled.js direction='end' n=1000000          ***     24.38 %       ±3.03%  ±4.01%  ±5.15%
timers/timers-timeout-nexttick.js n=50000                            **     -4.00 %       ±2.41%  ±3.18%  ±4.08%
timers/timers-timeout-unpooled.js n=1000000                         ***     -4.59 %       ±2.62%  ±3.45%  ±4.44%

@nodejs-github-bot
Copy link
Collaborator

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.

I'm not a particular fan of how this makes [kRefed] always be behind _destroyed. Seems like we are unnecessarily adding more "timer is alive or not" dependence to another semi-private property.

Is that required to be part of these changes?

lib/timers.js Show resolved Hide resolved
@Fishrock123
Copy link
Contributor

@apapirovski thoughts?

@apapirovski
Copy link
Member Author

@Fishrock123 Your comment is fair. Let's discuss in person at the summit. Didn't have time for the past month to think about this but will probably try to wrap up this PR in the next two days.

@Fishrock123
Copy link
Contributor

@apapirovski good to merge?

@apapirovski
Copy link
Member Author

@Fishrock123 yeah, should be. I’ll confirm this weekend and do it if so.

apapirovski and others added 3 commits December 13, 2019 15:35
Most of the code in insert is only applicable to scheduling
non-timers or re-scheduling timers. We can skip most of it
in the case of setTimeout, setInterval & setUnrefTimeout.
Expired timers were not being refresh correctly and
would always act as unrefed if refresh was called.
@apapirovski
Copy link
Member Author

apapirovski commented Dec 13, 2019

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 14, 2019

@apapirovski apapirovski added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 14, 2019
@BridgeAR
Copy link
Member

@apapirovski the latest benchmark results do not indicate a big difference at all and these are the only three benchmarks that seemed to have any significance:

01:16:52  timers/timers-depth.js n=1000                                       ***      0.71 %       ±0.19%  ±0.25%  ±0.32%
01:16:52  timers/timers-insert-unpooled.js direction='end' n=1000000            *     -2.55 %       ±2.40%  ±3.17%  ±4.07%
01:16:52  timers/timers-timeout-pooled.js n=10000000                           **     -5.74 %       ±4.14%  ±5.46%  ±7.02%

@Fishrock123
Copy link
Contributor

I think this change aims more to reduce potential edge-cases than desiring a perf impact?

@Fishrock123
Copy link
Contributor

Aside form an extra function closure the only thing I can think of is the coalescing boolean checks...

@Trott
Copy link
Member

Trott commented Dec 18, 2019

Should this be squashed into one commit or two? If two, which commit should the fixup commit go with?

@Trott
Copy link
Member

Trott commented Dec 21, 2019

Should this be squashed into one commit or two? If two, which commit should the fixup commit go with?

@apapirovski ^^^^^^^^

BridgeAR pushed a commit that referenced this pull request Dec 25, 2019
Most of the code in insert is only applicable to scheduling
non-timers or re-scheduling timers. We can skip most of it
in the case of setTimeout, setInterval & setUnrefTimeout.

PR-URL: #27345
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit that referenced this pull request Dec 25, 2019
Expired timers were not being refresh correctly and
would always act as unrefed if refresh was called.

PR-URL: #27345
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR
Copy link
Member

@Trott the fixup is for the second commit. I went ahead and landed it in two commits.

Landed in 0b89761, 1fab8a9 🎉

@BridgeAR BridgeAR closed this Dec 25, 2019
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
Most of the code in insert is only applicable to scheduling
non-timers or re-scheduling timers. We can skip most of it
in the case of setTimeout, setInterval & setUnrefTimeout.

PR-URL: #27345
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
Expired timers were not being refresh correctly and
would always act as unrefed if refresh was called.

PR-URL: #27345
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
targos pushed a commit that referenced this pull request Jan 14, 2020
Most of the code in insert is only applicable to scheduling
non-timers or re-scheduling timers. We can skip most of it
in the case of setTimeout, setInterval & setUnrefTimeout.

PR-URL: #27345
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jan 14, 2020
Expired timers were not being refresh correctly and
would always act as unrefed if refresh was called.

PR-URL: #27345
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jan 14, 2020
Most of the code in insert is only applicable to scheduling
non-timers or re-scheduling timers. We can skip most of it
in the case of setTimeout, setInterval & setUnrefTimeout.

PR-URL: #27345
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jan 14, 2020
Expired timers were not being refresh correctly and
would always act as unrefed if refresh was called.

PR-URL: #27345
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Most of the code in insert is only applicable to scheduling
non-timers or re-scheduling timers. We can skip most of it
in the case of setTimeout, setInterval & setUnrefTimeout.

PR-URL: #27345
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Expired timers were not being refresh correctly and
would always act as unrefed if refresh was called.

PR-URL: #27345
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants