rate limit auto-tuning #2899
rate limit auto-tuning #2899
Conversation
|
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
Can you share your benchmark results? |
|
I will provide some db_bench numbers. But keep in mind the real benefit should come in variable write rate scenarios, which I think db_bench doesn't support. |
|
|
|
Result if we make a change to reset rate limit whenever there's a period with too few rate limiter drains:
|
|
@ajkr has updated the pull request. View: changes, changes since last import |
|
Results with latest parameters. Command is the same but tested with high benchmark write rate as well (with 20M keys).
|
|
@ajkr has updated the pull request. View: changes, changes since last import |
|
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@ajkr has updated the pull request. View: changes, changes since last import |
util/rate_limiter.cc
Outdated
| std::chrono::microseconds(1)) / | ||
| std::chrono::microseconds(refill_period_us_); | ||
| int64_t drained_pct = | ||
| (num_drains_ - prev_num_drains_) * 100 / elapsed_intervals; |
There was a problem hiding this comment.
Either assert (elapsed_intervals > 0) or return if it is 0.
util/rate_limiter.cc
Outdated
| } else if (drained_pct < kLowWatermarkPct) { | ||
| new_bytes_per_sec = | ||
| std::max(max_bytes_per_sec_ / kAllowedRangeFactor, | ||
| prev_bytes_per_sec * 100 / (100 + kAdjustFactorPct)); |
There was a problem hiding this comment.
In this function, we need to care about overflow in general. For example, prev_bytes_per_sec * 100 can technically overflow. It is a possible that in somewhere else, the value of GetBytesPerSecond() has been sanitized so that it is not close to overflow here, but it's not super clear when I read this function. Can user set a very high rate limiter threshold, like max_int and it gets weird results here? It ever caused problem to rate limiter before. Some users think if they don't need to limit the rate, just set it to max_int. So I suggest it is explicitly handled in the code here.
ajkr
force-pushed
the
rate-limit-tuner2
branch
from
59c2d05
to
98eae46
Compare
|
@ajkr has updated the pull request. |
There was a problem hiding this comment.
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@ajkr is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
hi @ajkr I guess the update line at: https://github.com/facebook/rocksdb/blob/main/util/rate_limiter.cc#L407 should be: |
|
Good point, thanks for pointing it out. I forgot we ever tried to use the delta in drain count for tuning, and none of the docs suggest we do that, even the original announcement post (http://rocksdb.org/blog/2017/12/18/17-auto-tuned-rate-limiter.html). So I think now we can adopt the bug as part of the feature by deleting |
As reported in facebook#2899 (comment), `prev_num_drains_` is confusing as we never set it to nonzero. So this PR removes it. Test Plan: `make check -j24`
Summary: As reported in #2899 (comment), `prev_num_drains_` is confusing as we never set it to nonzero. So this PR removes it. Pull Request resolved: #9484 Test Plan: `make check -j24` Reviewed By: hx235 Differential Revision: D33923203 Pulled By: ajkr fbshipit-source-id: 6277d50a198b90646583ee8094c2e6a1bbdadc7b
Dynamic adjustment of rate limit according to demand for background I/O. It increases by a factor when limiter is drained too frequently, and decreases by the same factor when limiter is not drained frequently enough. The parameters for this behavior are fixed in
GenericRateLimiter::Tune. Other changes:Env*configurable for testingTest Plan:
make check