Commit ae827659 authored by Stepan Palamarchuk's avatar Stepan Palamarchuk Committed by Facebook Github Bot

Pass the nextTick into scheduleTimeoutImpl instead of recomputing it

Summary:
Currently, every timeout scheduling will compute the next tick twice:
* when deciding which bucket to use
* when actually scheduling next timeout in `scheduleNextTimeout`

This means that we may end up using a bigger nextTick in `scheduleNextTimeout` (if we were at a boundary of a tick and/or if there was a context switch). This means that if the timeout value is low and we will put it for `nextTick`, then it will end up waiting 2.5s (while we make one round over all slots).

With this change we make sure that the same tick is used.

I could consistently reproduce an issue by running 256 threads that just schedule immediate timeouts all the time.

Reviewed By: yfeldblum

Differential Revision: D13531907

fbshipit-source-id: a152cbed7b89e8426b2c52281b5b6e171e4520ea
parent 3a06d042
......@@ -104,8 +104,8 @@ HHWheelTimer::~HHWheelTimer() {
void HHWheelTimer::scheduleTimeoutImpl(
Callback* callback,
std::chrono::milliseconds timeout) {
auto nextTick = calcNextTick();
std::chrono::milliseconds timeout,
int64_t nextTick) {
int64_t due = timeToWheelTicks(timeout) + nextTick;
int64_t diff = due - nextTick;
CallbackList* list;
......@@ -146,13 +146,14 @@ void HHWheelTimer::scheduleTimeout(
count_++;
callback->setScheduled(this, timeout);
scheduleTimeoutImpl(callback, timeout);
auto nextTick = calcNextTick();
scheduleTimeoutImpl(callback, timeout, nextTick);
/* If we're calling callbacks, timer will be reset after all
* callbacks are called.
*/
if (!processingCallbacksGuard_) {
scheduleNextTimeout();
scheduleNextTimeout(nextTick);
}
}
......@@ -168,7 +169,7 @@ bool HHWheelTimer::cascadeTimers(int bucket, int tick) {
while (!cbs.empty()) {
auto* cb = &cbs.front();
cbs.pop_front();
scheduleTimeoutImpl(cb, cb->getTimeRemaining(getCurTime()));
scheduleTimeoutImpl(cb, cb->getTimeRemaining(getCurTime()), calcNextTick());
}
// If tick is zero, timeoutExpired will cascade the next bucket.
......@@ -235,7 +236,7 @@ void HHWheelTimer::timeoutExpired() noexcept {
return;
}
}
scheduleNextTimeout();
scheduleNextTimeout(calcNextTick());
}
size_t HHWheelTimer::cancelAll() {
......@@ -272,8 +273,7 @@ size_t HHWheelTimer::cancelAll() {
return count;
}
void HHWheelTimer::scheduleNextTimeout() {
auto nextTick = calcNextTick();
void HHWheelTimer::scheduleNextTimeout(int64_t nextTick) {
int64_t tick = 1;
if (nextTick & WHEEL_MASK) {
......
......@@ -209,9 +209,6 @@ class HHWheelTimer : private folly::AsyncTimeout,
* before scheduling the new timeout.
*/
void scheduleTimeout(Callback* callback, std::chrono::milliseconds timeout);
void scheduleTimeoutImpl(
Callback* callback,
std::chrono::milliseconds timeout);
/**
* Schedule the specified Callback to be invoked after the
......@@ -303,7 +300,11 @@ class HHWheelTimer : private folly::AsyncTimeout,
int64_t calcNextTick();
void scheduleNextTimeout();
void scheduleTimeoutImpl(
Callback* callback,
std::chrono::milliseconds timeout,
int64_t nextTick);
void scheduleNextTimeout(int64_t nextTick);
size_t cancelTimeoutsFromList(CallbackList& timeouts);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment