Skip to content

Commit

Permalink
Revert "server: Add memory check during eviction"
Browse files Browse the repository at this point in the history
This reverts commit 1cde153ceafb59901ad133317b85d357573cf2df.

Reason for revert: While this CL was attempting to address a
reasonable conern (under-eviction), it unfornately goes too far in the
opposite direction and over-evicts as well as unnecessarily burning
through additional CPU cycles.

As background, when we determine we need to evict some bytes in
`do_eviction()`, we send messages to domains requesting them to
evict some number of bytes. Those messsages are sent _asynchonously_,
and any reciprocating updates to the domain sizes are recieved
_asynchronously_.

This CL introduced a loop around that core eviction functionality,
assumably thinking that eviction is synchronous. As there is nothing
blocking or delaying each iteration of the loop, it would hammer away
sending async eviction messages until the domain sizes fell below the
threshold, but because we sent more eviction requests than necessary,
we over-evicted.

This is compounded by the call to `MemoryTracker::allocated_bytes()`
on each loop iteration. That function must update all jemalloc stats
by updating an epoch value inside of jemalloc, which turns out be an
expensive operation.

Change-Id: Id7cc5dec6da388d0ec7876e1e3259e2398272ca6
Reviewed-on: https://gerrit.readyset.name/c/readyset/+/7522
Tested-by: Buildkite CI
Reviewed-by: Ethan Donowitz <ethan@readyset.io>
  • Loading branch information
jasobrown-rs committed May 22, 2024
1 parent 712180b commit ecebda8
Showing 1 changed file with 3 additions and 4 deletions.
7 changes: 3 additions & 4 deletions readyset-server/src/worker/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -550,13 +550,13 @@ async fn do_eviction(
let span = info_span!("evicting");
let start = std::time::Instant::now();

let mut used: usize = memory_tracker.allocated_bytes()?;
let used: usize = memory_tracker.allocated_bytes()?;
gauge!(recorded::EVICTION_WORKER_HEAP_ALLOCATED_BYTES, used as f64);
// Are we over the limit?
match memory_limit {
None => Ok(()),
Some(limit) => {
while used >= limit {
if used >= limit {
// we are! time to evict.
// add current state sizes (could be out of date, as packet sent below is not
// necessarily received immediately)
Expand All @@ -583,6 +583,7 @@ async fn do_eviction(
let actual_over = used - limit;
let mut proportional_over =
((total_reported as f64 / used as f64) * actual_over as f64).round() as usize;

// here's how we're going to proceed.
// we don't want to _empty_ any views if we can avoid it.
// and we also need to be aware that evicting something from one place may cause a
Expand Down Expand Up @@ -660,8 +661,6 @@ async fn do_eviction(
domain_senders.remove(&target);
}
}
// Check again the allocated memory size
used = memory_tracker.allocated_bytes()?;
}
histogram!(
recorded::EVICTION_WORKER_EVICTION_TIME,
Expand Down

0 comments on commit ecebda8

Please sign in to comment.