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

use unsorted array in Timers._unrefActive() #2540

Closed
wants to merge 3 commits into from
Closed

use unsorted array in Timers._unrefActive() #2540

wants to merge 3 commits into from

Commits on Aug 31, 2015

  1. timers: Avoid linear scan in _unrefActive. 

    Before this change, _unrefActive would keep the unrefList sorted when
adding a new timer.

Because _unrefActive is called extremely frequently, this linear scan
(O(n) at worse) would make _unrefActive show high in the list of
contributors when profiling CPU usage.

This commit changes _unrefActive so that it doesn't try to keep the
unrefList sorted. The insertion thus happens in constant time.

However, when a timer expires, unrefTimeout has to go through the whole
unrefList because it's not ordered anymore.

It is usually not large enough to have a significant impact on
performance because:
- Most of the time, the timers will be removed before unrefTimeout is
  called because their users (sockets mainly) cancel them when an I/O
  operation takes place.
- If they're not, it means that some I/O took a long time to happen, and
  the initiator of subsequents I/O operations that would add more timers
  has to wait for them to complete.

With this change, _unrefActive does not show as a significant
contributor in CPU profiling reports anymore.

Fixes: nodejs/node-v0.x-archive#8160
PR-URL: nodejs/node-v0.x-archive#8751

Signed-off-by: Timothy J Fontaine <tjfontaine@gmail.com>

Conflicts:
	lib/timers.js
    @Fishrock123
    Julien Gilli authored and Fishrock123 committed Aug 31, 2015
    Configuration menu
    Copy the full SHA
    8ae5292 View commit details
    Browse the repository at this point in the history

  2. timers: don't mutate unref list while iterating it 

    Commit 934bfe2 had introduced a
regression where node would crash trying to access a null unref timer if
a given unref timer's callback would remove other unref timers set to
fire in the future.

More generally, it makes the unrefTimeout function more solid by not
mutating the unrefList while traversing it.

Fixes: nodejs/node-v0.x-archive#8897
PR-URL: nodejs/node-v0.x-archive#8905

Reviewed-By: Timothy J Fontaine <tjfontaine@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Conflicts:
	lib/timers.js
    @Fishrock123
    Julien Gilli authored and Fishrock123 committed Aug 31, 2015
    Configuration menu
    Copy the full SHA
    27b5b0e View commit details
    Browse the repository at this point in the history

  3. timers: minor _unrefActive fixes and improvements 

    This commit addresses most of the review comments in
nodejs#2540, which are kept in this
separate commit so as to better preserve the prior two patches as they
landed in 0.12.

This commit:
- Fixes a bug with unrefActive timers and disposed domains.
- Fixes a bug with unrolling an unrefActive timer from another.
- Adds a test for both above bugs.
- Improves check logic, making it stricter, simpler, or both.
- Optimizes nicer with a smaller, separate function for the try/catch.
    @Fishrock123
    Fishrock123 committed Aug 31, 2015
    Configuration menu
    Copy the full SHA
    8104759 View commit details
    Browse the repository at this point in the history