-
Notifications
You must be signed in to change notification settings - Fork 24
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
PLAT-937: added TimerEventSource, enabling use of a single fd for mul… #197
base: master
Are you sure you want to change the base?
Conversation
addTimer(double delay, const OnTick & onTick) | ||
{ | ||
Date now = Date::now(); | ||
uint64_t timerId = counter_.fetch_add(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wraparound?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeremybarnes indeed and this raises a design question that I worked-around by using an assertion... I do not see how addTimer can guarantee that the ids will be unique, other than by remembering them somehow. I considered the use of a guid instead, but even though it postpones it further in time, it does not solve the problem either.
Sorry... the JIRA message got lost in my spam. Looks fine to me. +1 once my comments are addressed. Didn't we already have this functionality somewhere? |
I wonder what is the use case where this code is useful? The kernel probably implements something similar internally. It looks like an optimization so a bench of the problem and the solution would be very useful in understanding why this was (is it?) necessary. |
@EricRobert the reason for this code is to implement the functionality of "PeriodicEventSource" with a single file descriptor, rather than with a file descriptor per alarm. You can view it as a "PeriodicEventSource" version 2. |
@jeremybarnes for something similar: I don't recall where, except in PeriodicEventSource, which uses 1 fd per alarm. This class is meant to be instantiated by MessageLoop only and used via its "addTimer" method. |
auto loc = lower_bound(timerQueue_.begin(), timerQueue_.end(), | ||
timer, timerCompare); | ||
timerQueue_.insert(loc, move(timer)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This kind of thing is normally implemented using a min-heap. Using a vector for this introduces a lot of degenerate cases especially since the goal of this is to support a lot of timers i.e. if the number of timers is small, why bother? if it's large, it's likely not very efficient unless all timers are inserted at the end.
…tiple timers