-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
Break stalls when no bg work is happening #1884
Conversation
@IslamAbdelRahman has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
@IslamAbdelRahman updated the pull request - view changes - changes since last import |
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.
LGTM!
@IslamAbdelRahman has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
db/db_impl.cc
Outdated
} | ||
|
||
delayed = true; | ||
bg_cv_.TimedWait(static_cast<int>(stall_end - now)); |
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.
Can we still sleep a small amount of time? One reason I'm wary is to add one another use of TimedWait(). I recently realize that there may be corner case that a clock change may cause the wait not accurate. I think a shorter sleep will make it easier to read too.
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.
@siying, I think you mean there is a problem with NowMicros() not TimedWait(), correct ?
NowMicros() is the one that will be affected by system time change, correct ?
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.
@IslamAbdelRahman no I mean TimedWait(). I suspect inside pthread's wait until call, I think it calls non-monotonic time to determine duration to wait for.
db/db_impl.cc
Outdated
// We will delay the write until the wall clock reach stall_end or | ||
// we don't have any flushes or compactions running in the bg | ||
uint64_t stall_end = sw.start_time() + delay; | ||
while (bg_flush_scheduled_ > 0 || bg_compaction_scheduled_ > 0) { |
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.
What's the reason not callling write_controller_.NeedsDelay()?
Hey @siying, I think you missed my last comment here |
e0c6b5c
to
44701ad
Compare
@IslamAbdelRahman updated the pull request - view changes - changes since last import |
Adress @siying comments
|
db/db_impl.cc
Outdated
const uint64_t kDelayInterval = 10000; | ||
uint64_t stall_end = stall_start + (delay * std::milli::den); | ||
while (write_controller_.NeedsDelay()) { | ||
if (env_->NowNanos() >= stall_end) { |
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.
I didn't mean we need to call a monotonic clock here. I just want to avoid TimedWait() if possible. But what you are doing here is still correct.
@IslamAbdelRahman has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
trying to figure out why the test is failing now |
44701ad
to
bc89608
Compare
@IslamAbdelRahman updated the pull request - view changes - changes since last import |
@IslamAbdelRahman has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Current stall will keep sleeping even if there is no Flush/Compactions to wait for, I changed the logic to break the stall if we are not flushing or compacting
db_bench command used