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

Refactor Timers #4007

Closed
wants to merge 29 commits into from
Closed

Conversation

Fishrock123
Copy link
Contributor

Commit 1

Consolidates the implementation of regular and internal (_unrefActive)
timers.

Includes an optimization for listOnTimeout() that previously only
internal timers had. (_runOnTimeout)

Also includes some minor other cleanup.

Commit 2

Describes the How and Why of the timers implementation, as well as
adding comments in spots that should allow for an easier understanding
of what is going on.

The timers implementation is very efficient, at a cost.
That cost is readable understandability, and this aims to improve that.


cc @bnoordhuis / @piscisaureus / @trevnorris / @misterdjules / etc?

This attempts to improve the timers implementation by consolidating the internal and regular timer logic.

I have not yet run performance testing / profiling on this, so feel free to help me out there if you are able to but I will hopefully be able to get to it shortly.

Tests pass locally, CI: https://ci.nodejs.org/job/node-test-commit/1237/

cc @nodejs/documentation and @nodejs/inclusivity I'd like thoughts on the format of the comments and their content and if they fit in well as code comments, as well as how understandable this is to people who are unfamiliar with the code.

@Fishrock123 Fishrock123 added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Nov 24, 2015
list = new Timer();
if (unrefed) list.unref();
list._unrefed = unrefed;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is possibly a better way of doing this.

In a nutshell we need to be able to tell if a specific list (TimerWrap) is (supposed to be) unrefed or not.

I'm not to worried about people messing with these top-level handles from C++ though.

Choose a reason for hiding this comment

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

Could just store a reference to lists so just have one-line delete instead of branch down there?

Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure you add _unrefed = false or similar to the Timer constructor, or you're mutating the object map.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexlamsl like maybe _parent?

@trevnorris good catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, that doesn't apply to Timer, this sets it on the list not the timer itself. :)

Choose a reason for hiding this comment

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

@Fishrock123 yes _parent sounds good, then down below you can do delete list._parent[msecs]; instead of

if (list._unrefed) {  
  delete unrefedLists[msecs];  
} else {  
  delete refedLists[msecs];  
}  

Choose a reason for hiding this comment

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

@Fishrock123 isn't list a Timer object, as declared a few lines above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexlamsl Ah, I thought he was referring to Timeout. This is a TimerWrap, imported as Timer.

@trevnorris were you referring to Timer, as in TimerWrap, or Timeout?

Copy link
Contributor

Choose a reason for hiding this comment

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

list = new Timer();  // <- whatever object is returned from here

EDIT: nm. forgot that Timer comes from node::TimerWrap::New() in src/timer_wrap.cc.

@Fishrock123
Copy link
Contributor Author

@sup & @beaugunderson Thanks, Atom's spelling detection isn't very good unfortunately. :(

@ghost
Copy link

ghost commented Nov 24, 2015

@Fishrock123 overall, i have to say i'm very impressed. if only all lib files were documented this well...

the only thing i'd suggest is maybe having a kind of difficulty rating referring to how complex and hard to understand the code and the explaining comments are. sadly, you can't easily explain some things :(

but that's just an idea, and would maybe make more sense if, well, all lib files were documented this well.

LGTM once the spelling errors are fixed ^^


// Object maps containing linked lists of timers, keyed and sorted by their
// duration in miliseconds.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: mili → milli

@Trott
Copy link
Member

Trott commented Nov 24, 2015

This is awesome. The resulting code is much easier to understand. The comments are helpful too.

const msecs = item._idleTimeout;
if (msecs < 0 || msecs === undefined) return;

item._idleStart = Timer.now();

var list;
const lists = unrefed ? unrefedLists : refedLists;

Choose a reason for hiding this comment

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

Why not var list = lists[msecs] and save a branch clause below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might not exist yet, so we need to be able to check that make a new one if it does not exist yet.

Choose a reason for hiding this comment

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

What I meant is instead of

var list;
const lists = ...;
if (lists[msecs]) {
  list = lists[msecs];
} else {
  ...
}

How about

const lists = ...;
var list = lists[msecs];
if (!list) {
  ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that sounds good, yeah.

@Fishrock123
Copy link
Contributor Author

// The timers are distributed into separate lists by milliseconds primarly to
// reduce the impact of the linked list's O(n) (linear) insertion time.

This is incorrect, I am wrong. @piscisaureus was right about the efficiency of the regular timers impl.

Heck, even after all of this I still didn't clue 100% into why this was supposed to be so efficient.

insert() doesn't actually insert, it only appends to the linked list. That is, it follows the libev guide closer than I had previously understood.

Example:

You only ever need to append to a list of timers scheduled for 50ms because even if you schedule two timers at the same time, none will ever be sooner than that timeout time, or an existing timeout, within that 50ms list.

On timeout, we only need to check the start of the list for timers that need to timeout. This is because all of those timers will only ever have a 50ms timeout, and so any timers past the first one that we don't need to timeout yet will also have a due date later than the current timeout, since they are all 50ms and must have been scheduled later than we first timed out.

@Fishrock123
Copy link
Contributor Author

I have now updated the PR with comments now correctly describing how exactly it works.

All operations in the JavaScript layer are virtually constant time. What we have effectively acts as a timer wheel. I do not think it is possible to make a better overall implementation.

// is complete, but not by using whatever domain was left over
// when the timeout threw its exception.
var oldDomain = process.domain;
process.domain = null;

Choose a reason for hiding this comment

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

At first sight, it seems that not setting process.domain to null would break test/parallel/test-domain-exit-dispose-again.js now that #3990 landed.

@Fishrock123
Copy link
Contributor Author

@misterdjules hmmm, let's find out:

CI rebased onto master: https://ci.nodejs.org/job/node-test-pull-request/866/

@misterdjules
Copy link

@Fishrock123 Confirmed.

@Fishrock123
Copy link
Contributor Author

if (!timer._onTimeout) continue;

var domain = timer.domain;
if (domain) {

// v0.4 compatibility: if the timer callback throws and the

Choose a reason for hiding this comment

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

This comment is obsolete, and doesn't describe the current state of the code base. It seems that this PR is a good opportunity to remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted, I'll take a look at it in detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, the comment was originally added in b221fe9 and then updated for domains in 4401bb4. However, it used to continue.

I think I may have made a mistake here having it return as it it did in the original _unrefActive(), perhaps we are missing a test for this?

Fishrock123 added a commit to Fishrock123/node-perf-results that referenced this pull request Nov 30, 2015
@rvagg
Copy link
Member

rvagg commented Feb 27, 2016

So good, well done on making it to landing @Fishrock123! I now crown you the new king of timers 👑, we know who to send people to when they have questions.

@jasnell
Copy link
Member

jasnell commented Mar 2, 2016

In generally I think this should actually be safe for both LTS and v5 but I'd like to see it sit for a bit to be sure there are no hidden regressions in here (there shouldn't be, I just prefer to be conservative about it).

@jasnell
Copy link
Member

jasnell commented Mar 2, 2016

oh, and yes, great job @Fishrock123 ... this is good stuff.

evanlucas pushed a commit that referenced this pull request Mar 14, 2016
Consolidates the implementation of regular and internal (_unrefActive)
timers.

Also includes a couple optimizations:
- Isolates the try/catch from listOnTimeout() in a new tryOnTimeout().
- Uses a TimersList constructor as the base for linkedlists.

Additionally includes other cleanup and clarification, such as a rename
of "Timer" to "TimerWrap".

PR-URL: #4007
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Julien Gilli <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
evanlucas pushed a commit that referenced this pull request Mar 14, 2016
Describes the How and Why of the timers implementation, as well as
adding comments in spots that should allow for an easier understanding
about what is going on.

The timers implementation is very efficient, at a cost.
That cost is readable understandability, and this aims to improve that.

PR-URL: #4007
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Julien Gilli <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
@evanlucas evanlucas mentioned this pull request Mar 14, 2016
rvagg pushed a commit that referenced this pull request Mar 16, 2016
Consolidates the implementation of regular and internal (_unrefActive)
timers.

Also includes a couple optimizations:
- Isolates the try/catch from listOnTimeout() in a new tryOnTimeout().
- Uses a TimersList constructor as the base for linkedlists.

Additionally includes other cleanup and clarification, such as a rename
of "Timer" to "TimerWrap".

PR-URL: #4007
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Julien Gilli <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
rvagg pushed a commit that referenced this pull request Mar 16, 2016
Describes the How and Why of the timers implementation, as well as
adding comments in spots that should allow for an easier understanding
about what is going on.

The timers implementation is very efficient, at a cost.
That cost is readable understandability, and this aims to improve that.

PR-URL: #4007
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Julien Gilli <[email protected]>
Reviewed-By: Chris Dickinson <[email protected]>
@MylesBorins
Copy link
Contributor

@Fishrock123 as v6 is getting quite close to becoming LTS I'm going to opt to keep these changes out of the v4 release line.

This is in no ways a final decision and if you believe they should land we should discuss it in the next LTS meeting. Thanks for the hard work on these changes!

@Fishrock123
Copy link
Contributor Author

@thealphanerd While this looks large, the impact on running programs is minimal but mostly positive.
¯_(ツ)_/¯

@MylesBorins
Copy link
Contributor

@Fishrock123 would you be willing to open an issue on the LTS repo about the various timers changes for v4?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.