Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

timers: Avoid linear scan in _unrefActive. #8225

Closed
wants to merge 1 commit into from

Conversation

misterdjules
Copy link

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 #8160.

startTimer();

setTimeout(function () {
assert(nbTimeouts === N);
Copy link
Member

Choose a reason for hiding this comment

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

Minor style issues: function() { and two space indent, not four. You could use assert.equal() here.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you, PR updated!

@bnoordhuis
Copy link
Member

I haven't looked too closely yet but what I've seen so far looks reasonable to me.

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#8160.
@misterdjules
Copy link
Author

@bnoordhuis @tjfontaine Just for the record, I don't think this PR should be merged before investigations about various implementation candidates are done.

@bnoordhuis
Copy link
Member

@misterdjules Great writeup! Let me offer my EUR .02:

  • An unordered array leaves you vulnerable to denial-of-service attacks. I'm not sure if you can realistically exploit that now but I wouldn't take chances.
  • A heap has O(1) expiry but O(log n) insertion and removal, which might get expensive when the operations are repeated often.

I originally suggested a timer wheel because it has O(1) insertion, removal and expiry. However, you have to step the wheel on every iteration of the event loop and while that's an O(1) operation, it does mean that actual performance may be worse. We'd have to benchmark it to know for sure.

@misterdjules
Copy link
Author

@bnoordhuis Thank you for reading! I agree with both points.

I updated the original issue with the link to the wiki page. My recommendations would be to either integrate the heap implementation and then benchmark the timer wheel, or wait before the timer wheel benchmark is done.

Anyway, I would not recommend using the unordered list implementation that this PR represents, so I would recommend closing it.

@bnoordhuis
Copy link
Member

Oh, I agree - a priority heap is a good first step and probably easier to integrate than a timer wheel.

I wonder - and this is just idle musing - whether a heap is more expensive for small sets of timers. Intuitively, O(n) insertion + O(1) removal should be more efficient for small values of n than O(log n) insertion and removal. That assumes that the O(n) insertion averages out to O(n / 2) in practice, which should be the case with a generally randomized distribution.

Like I said, idle musing - it's unlikely to matter in the grand scheme of things.

@misterdjules
Copy link
Author

@bnoordhuis Right, that's valid for values of n that do not really concern us performance-wise.

@indutny
Copy link
Member

indutny commented Aug 27, 2014

@misterdjules closing this?

@misterdjules
Copy link
Author

Closing it. We will probably use the heap implementation instead, or a timer wheel depending on how we decide to move forward with this. See this Wiki page for more details.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants