server: add scaled timer utility class #11773
Conversation
This class produces Envoy::Event::Timer objects whose durations can be scaled en-masse by adjusting the scale factor on the manager. This class will be used to implement timer scaling for the OverloadManagerImpl based on load. Signed-off-by: Alex Konradi <akonradi@google.com>
|
/assign antoniovicente |
| std::vector<TimerPtr> timers; | ||
| for (int i = 1; i <= 3; i++) { | ||
| timers.push_back(manager.createTimer([i, &cb] { cb.Call(i); })); | ||
| timers.back()->enableTimer(std::chrono::seconds(i)); |
There was a problem hiding this comment.
Add a test to cover the case of multiple timers scheduled at the exact same time. I think you'll find some problems with registration and some timers not firing when expected. std::set treats the following condition as equality:
!(a < b) && !(b < a)
You need a secondary comparison criteria for timers that have the same time. Possible implementation:
bool ScaledTimerManager::ScaledTimerList::Compare::operator()(const ScaledTimerImpl* lhs,
const ScaledTimerImpl* rhs) const {
if (targetTime(lhs) == targetTime(rhs)) {
return lhs < rhs;
}
return targetTime(lhs) < targetTime(rhs);
}
| disableTimer(); | ||
| } | ||
| active_.emplace(ActiveTimer{ | ||
| .start = manager_.dispatcher_.timeSource().monotonicTime(), |
There was a problem hiding this comment.
Use of this C99 syntax in C++ builds requires the c++20 standard which AFAIK is still pending ratification and is not enabled yet by the Microsoft Windows compiler. See #11316 (comment)
| } | ||
|
|
||
| void ScaledTimerManager::ScaledTimerImpl::enableTimer(const std::chrono::milliseconds& delay, | ||
| const ScopeTrackedObject* object) { |
There was a problem hiding this comment.
nit: avoid code duplication.
Replace implementation with:
enableHRTimer(delay, object);
or some common method like TimerImpl does.
| if (target_delay > std::chrono::microseconds::zero()) { | ||
| dispatch_timer_->enableHRTimer(target_delay); | ||
| } else { | ||
| dispatch_timer_->enableHRTimer(std::chrono::microseconds::zero()); |
There was a problem hiding this comment.
I have an upcoming change that will break this since 0msec timer scheduling will no longer happen in the same iteration as this call. But the breakage is more fundamental: 0ms timers scheduled via this interface instead of the regular one would execute in the current iteration instead of the next iteration due to the way they are merged with other timers. I need to think of a way to achieve the right semantics; it may require keeping new additions separate and use of an update callback to move the newly added callbacks to the main set and adjust dispatch_timer_
| void ScaledTimerManager::ScaledTimerImpl::enableTimer(const std::chrono::milliseconds& delay, | ||
| const ScopeTrackedObject* object) { | ||
| if (active_.has_value()) { | ||
| disableTimer(); |
There was a problem hiding this comment.
What are the consequences of skipping this disableTimer call?
Could use some comments regarding the importance of removing the timer before any changes to active_.
| void ScaledTimerManager::dispatchCallback() { | ||
| // Find all timers that are ready to be handled and dispatch them. | ||
| const auto now = dispatcher_.timeSource().monotonicTime(); | ||
| for (auto timer = timers_.pop(now); timer.has_value(); timer = timers_.pop(now)) { |
There was a problem hiding this comment.
Consider this instead:
do {
auto timer = timers_.pop(now);
if (!timer.has_value()) {
break;
}
timer.value()->active_.reset();
timer.value()->dispatch(dispatcher_);
} while(true);
|
|
||
| void ScaledTimerManager::ScaledTimerList::setScaleFactor(double scale_factor) { | ||
| std::set<ScaledTimerImpl*, Compare> reordered(timers_.begin(), timers_.end(), | ||
| Compare{.scale_factor = scale_factor}); |
There was a problem hiding this comment.
Doing a linear scan and O(nlgn) set insertion computation to reorder entries when the scale factor changes seems very expensive. I wonder what alternate approaches we could consider to avoid this expense.
|
|
||
| // Timer impl: | ||
| void enableTimer(const std::chrono::milliseconds& delay, | ||
| const ScopeTrackedObject* object) override; |
There was a problem hiding this comment.
We talked about the issue of this timer scaling down to 0, not down to a specified minimum. I'm not sure that the Timer interface applies for scaled timers.
| } | ||
|
|
||
| // Each duration will be treated as if it was 10% of the original requested value. | ||
| manager.setDurationScaleFactor(0.1); |
There was a problem hiding this comment.
Cover unusual cases like setDurationScaleFactor(0.0);
| } | ||
|
|
||
| void ScaledTimerManager::setDurationScaleFactor(double scale_factor) { | ||
| timers_.setScaleFactor(scale_factor); |
There was a problem hiding this comment.
ASSERT(scale_factor <= 1.0 && scale_factor >= 0.0);
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
bot
added
the
stale
|
This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Commit Message:
Add a ScaledTimer utility class for producing Envoy::Event::Timer objects whose durations can be scaled en-masse by adjusting the scale factor on the manager. This class will be used to implement timer scaling for the OverloadManagerImpl based on load.
Additional Description:
Risk Level: low - the code is tested but not integrated
Testing: wrote unit test
Docs Changes: none
Release Notes: none
#11427
/cc @antoniovicente