Use monotonic time points in write_controller.cc and rate_limiter.cc #1865
Conversation
include/rocksdb/env.h
Outdated
| virtual uint64_t NowMicrosMonotonic() { | ||
| return std::chrono::duration_cast<std::chrono::microseconds>( | ||
| std::chrono::steady_clock::now().time_since_epoch()) | ||
| .count(); |
There was a problem hiding this comment.
Can you follow the convention to rely on specific implementation for this? Just leave this one as NowNanos() / 1000.
util/rate_limiter.cc
Outdated
| @@ -107,7 +107,10 @@ void GenericRateLimiter::Request(int64_t bytes, const Env::IOPriority pri) { | |||
| (!queue_[Env::IO_LOW].empty() && | |||
| &r == queue_[Env::IO_LOW].front()))) { | |||
| leader_ = &r; | |||
| timedout = r.cv.TimedWait(next_refill_us_); | |||
| int64_t delta = next_refill_us_ - env_->NowMicrosMonotonic(); | |||
| delta = delta > 0 ? delta : 0; | |||
There was a problem hiding this comment.
Why do you still need to wait if delta is 0?
xiaofeidu008
force-pushed
the
NowMicrosMonotonic
branch
from
9b454e0
to
c2561cc
Compare
|
@xiaofeidu008 updated the pull request - view changes |
xiaofeidu008
force-pushed
the
NowMicrosMonotonic
branch
from
c2561cc
to
b1ee0ce
Compare
|
@xiaofeidu008 updated the pull request - view changes |
xiaofeidu008
changed the title
|
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
| @@ -61,6 +61,9 @@ class GenericRateLimiter : public RateLimiter { | |||
| private: | |||
| void Refill(); | |||
| int64_t CalculateRefillBytesPerPeriod(int64_t rate_bytes_per_sec); | |||
| uint64_t NowMicrosMonotonic(Env* env) { | |||
| return env->NowNanos() / std::milli::den; | |||
| } | |||
There was a problem hiding this comment.
Ideally the two NowMicrosMonotonic() functions should be consolidated as one helper function. We don't have a good place to place it. If you want, you can create util/env_util.h to host a file there. If it is too complicated to you, you can leave it as it is.
There was a problem hiding this comment.
I looked for that place and could not find one when creating the diff. This is a one-liner function (kinda like a macro). So it's fine to leave it as it is. In the future, if the number of these utility functions grow, we should add some utility files.
summary: NowMicros() provides non-monotonic time. When wall clock is synchronized or changed, the non-monotonicity time points will affect write rate controllers. This patch changes write_controller.cc and rate_limiter.cc to use monotonic time points.
xiaofeidu008
force-pushed
the
NowMicrosMonotonic
branch
from
b1ee0ce
to
a046870
Compare
|
@xiaofeidu008 updated the pull request - view changes - changes since last import |
|
@siying I have added comment for NowNanos(). You may merge anytime you want. I don't think I have permission to merge :-) |
|
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
summary: NowMicros() provides non-monotonic time. When wall clock is
synchronized or changed, the non-monotonicity time points will affect write rate
controllers. This patch changes write_controller.cc and rate_limiter.cc to use
monotonic time points.