From c7f67cd76fec1ca77c1f525eeaf521a2ae36935c Mon Sep 17 00:00:00 2001 From: Tatsuya Kawano Date: Sun, 14 Apr 2024 10:29:04 +0800 Subject: [PATCH 1/9] Make `run_pending_tasks` to evict more entries from the cache at once - Disable the eviction batch size when the eviction listener is not set to the cache. - So single `run_pending_tasks` call should remove all entries that can be evicted from the cache. - Add a hard-coded maintenance task timeout duration. - When the eviction listener is set, `run_pending_tasks` should stop (return) after the maintenance task timeout is elapsed. - This is a safe-guard to prevent the maintenance task from running long time when the user wrote a slow eviction listener. - In older versions, the batch size was used to limit the time spent on the maintenance task. - Not that `run_pending_tasks` checks the timeout only after processing a batch of entries, so `run_pending_tasks` can run longer than the timeout duration. - Reduce the size of the read op and write op channels. --- src/common/concurrent/constants.rs | 16 +- src/common/concurrent/housekeeper.rs | 4 +- src/future/base_cache.rs | 294 ++++++++++++++++----------- src/future/housekeeper.rs | 73 +++++-- src/sync_base/base_cache.rs | 48 ++--- 5 files changed, 263 insertions(+), 172 deletions(-) diff --git a/src/common/concurrent/constants.rs b/src/common/concurrent/constants.rs index b4a7abb4..242b10cf 100644 --- a/src/common/concurrent/constants.rs +++ b/src/common/concurrent/constants.rs @@ -1,11 +1,19 @@ pub(crate) const MAX_SYNC_REPEATS: usize = 4; pub(crate) const PERIODICAL_SYNC_INITIAL_DELAY_MILLIS: u64 = 300; -pub(crate) const READ_LOG_FLUSH_POINT: usize = 512; -pub(crate) const READ_LOG_SIZE: usize = READ_LOG_FLUSH_POINT * (MAX_SYNC_REPEATS + 2); +pub(crate) const READ_LOG_FLUSH_POINT: usize = 64; +pub(crate) const READ_LOG_SIZE: usize = READ_LOG_FLUSH_POINT * (MAX_SYNC_REPEATS + 2); // 384 -pub(crate) const WRITE_LOG_FLUSH_POINT: usize = 512; -pub(crate) const WRITE_LOG_SIZE: usize = WRITE_LOG_FLUSH_POINT * (MAX_SYNC_REPEATS + 2); +pub(crate) const WRITE_LOG_FLUSH_POINT: usize = 64; +pub(crate) const WRITE_LOG_SIZE: usize = WRITE_LOG_FLUSH_POINT * (MAX_SYNC_REPEATS + 2); // 384 + +// TODO: Calculate the batch size based on the number of entries in the cache (or an +// estimated number of entries to evict) +pub(crate) const EVICTION_BATCH_SIZE: usize = WRITE_LOG_SIZE; +pub(crate) const INVALIDATION_BATCH_SIZE: usize = WRITE_LOG_SIZE; + +/// The default timeout duration for the `run_pending_tasks` method. +pub(crate) const DEFAULT_RUN_PENDING_TASKS_TIMEOUT_MILLIS: u64 = 100; #[cfg(feature = "sync")] pub(crate) const WRITE_RETRY_INTERVAL_MICROS: u64 = 50; diff --git a/src/common/concurrent/housekeeper.rs b/src/common/concurrent/housekeeper.rs index efb1f925..185eff52 100644 --- a/src/common/concurrent/housekeeper.rs +++ b/src/common/concurrent/housekeeper.rs @@ -36,11 +36,11 @@ impl Default for Housekeeper { impl Housekeeper { pub(crate) fn should_apply_reads(&self, ch_len: usize, now: Instant) -> bool { - self.should_apply(ch_len, READ_LOG_FLUSH_POINT / 8, now) + self.should_apply(ch_len, READ_LOG_FLUSH_POINT, now) } pub(crate) fn should_apply_writes(&self, ch_len: usize, now: Instant) -> bool { - self.should_apply(ch_len, WRITE_LOG_FLUSH_POINT / 8, now) + self.should_apply(ch_len, WRITE_LOG_FLUSH_POINT, now) } #[inline] diff --git a/src/future/base_cache.rs b/src/future/base_cache.rs index edc71fbb..30b244bf 100644 --- a/src/future/base_cache.rs +++ b/src/future/base_cache.rs @@ -12,7 +12,8 @@ use crate::{ concurrent::{ atomic_time::AtomicInstant, constants::{ - READ_LOG_FLUSH_POINT, READ_LOG_SIZE, WRITE_LOG_FLUSH_POINT, WRITE_LOG_SIZE, + EVICTION_BATCH_SIZE, INVALIDATION_BATCH_SIZE, READ_LOG_FLUSH_POINT, READ_LOG_SIZE, + WRITE_LOG_FLUSH_POINT, WRITE_LOG_SIZE, }, deques::Deques, entry_info::EntryInfo, @@ -177,6 +178,7 @@ where } else { (READ_LOG_SIZE, WRITE_LOG_SIZE) }; + let is_eviction_listener_enabled = eviction_listener.is_some(); let (r_snd, r_rcv) = crossbeam_channel::bounded(r_size); let (w_snd, w_rcv) = crossbeam_channel::bounded(w_size); @@ -202,7 +204,7 @@ where write_op_ch: w_snd, interrupted_op_ch_snd: i_snd, interrupted_op_ch_rcv: i_rcv, - housekeeper: Some(Arc::new(Housekeeper::default())), + housekeeper: Some(Arc::new(Housekeeper::new(is_eviction_listener_enabled))), } } @@ -881,6 +883,7 @@ where struct EvictionState<'a, K, V> { counters: EvictionCounters, notifier: Option<&'a Arc>>, + more_entries_to_evict: bool, } impl<'a, K, V> EvictionState<'a, K, V> { @@ -892,6 +895,7 @@ impl<'a, K, V> EvictionState<'a, K, V> { Self { counters: EvictionCounters::new(entry_count, weighted_size), notifier, + more_entries_to_evict: false, } } @@ -899,7 +903,7 @@ impl<'a, K, V> EvictionState<'a, K, V> { self.notifier.is_some() } - async fn add_removed_entry( + async fn notify_entry_removal( &mut self, key: Arc, entry: &TrioArc>, @@ -908,10 +912,10 @@ impl<'a, K, V> EvictionState<'a, K, V> { K: Send + Sync + 'static, V: Clone + Send + Sync + 'static, { - debug_assert!(self.is_notifier_enabled()); - if let Some(notifier) = self.notifier { notifier.notify(key, entry.value.clone(), cause).await; + } else { + panic!("notify_entry_removal is called when the notification is disabled"); } } } @@ -1386,21 +1390,6 @@ where } } -// TODO: Calculate the batch size based on the number of entries in the cache (or an -// estimated number of entries to evict) - -#[cfg(feature = "unstable-debug-counters")] -mod batch_size { - pub(crate) const EVICTION_BATCH_SIZE: usize = 10_000; - pub(crate) const INVALIDATION_BATCH_SIZE: usize = 10_000; -} - -#[cfg(not(feature = "unstable-debug-counters"))] -mod batch_size { - pub(crate) const EVICTION_BATCH_SIZE: usize = 500; - pub(crate) const INVALIDATION_BATCH_SIZE: usize = 500; -} - #[async_trait] impl InnerSync for Inner where @@ -1408,8 +1397,9 @@ where V: Clone + Send + Sync + 'static, S: BuildHasher + Clone + Send + Sync + 'static, { - async fn run_pending_tasks(&self, max_repeats: usize) { - self.do_run_pending_tasks(max_repeats).await; + /// Runs the pending tasks. Returns `true` if there are more entries to evict. + async fn run_pending_tasks(&self, max_repeats: usize, timeout: Option) -> bool { + self.do_run_pending_tasks(max_repeats, timeout).await } /// Notifies all the async tasks waiting in `BaseCache::schedule_write_op` method @@ -1429,24 +1419,30 @@ where V: Clone + Send + Sync + 'static, S: BuildHasher + Clone + Send + Sync + 'static, { - async fn do_run_pending_tasks(&self, max_repeats: usize) { + /// Runs the pending tasks. Returns `true` if there are more entries to evict. + async fn do_run_pending_tasks(&self, max_repeats: usize, timeout: Option) -> bool { if self.max_capacity == Some(0) { - return; + return false; } // Acquire some locks. let mut deqs = self.deques.lock().await; let mut timer_wheel = self.timer_wheel.lock().await; + let started_at = if timeout.is_some() { + Some(self.current_time_from_expiration_clock()) + } else { + None + }; let mut calls = 0; let current_ec = self.entry_count.load(); let current_ws = self.weighted_size.load(); let mut eviction_state = EvictionState::new(current_ec, current_ws, self.removal_notifier.as_ref()); - let mut should_process_logs = true; + loop { + calls += 1; - while should_process_logs && calls <= max_repeats { let r_len = self.read_op_ch.len(); if r_len > 0 { self.apply_reads(&mut deqs, &mut timer_wheel, r_len).await; @@ -1464,73 +1460,91 @@ where self.enable_frequency_sketch(&eviction_state.counters).await; } - let w_len = self.write_op_ch.len(); - // If there are any async tasks waiting in `BaseCache::schedule_write_op` // method for the write op channel to have enough room, notify them. let listeners = self.write_op_ch_ready_event.total_listeners(); if listeners > 0 { - let n = listeners.min(WRITE_LOG_SIZE - w_len); + let n = listeners.min(WRITE_LOG_SIZE - self.write_op_ch.len()); // Notify the `n` listeners. The `notify` method accepts 0, so no // need to check if `n` is greater than 0. self.write_op_ch_ready_event.notify(n); } - calls += 1; - should_process_logs = - self.read_op_ch.len() >= READ_LOG_FLUSH_POINT || w_len >= WRITE_LOG_FLUSH_POINT; - } + // Set this flag to `false`. The `evict_*` and `invalidate_*` methods + // below may set it to `true` if there are more entries to evict in next + // loop. + eviction_state.more_entries_to_evict = false; - if timer_wheel.is_enabled() { - self.evict_expired_entries_using_timers( - &mut timer_wheel, - &mut deqs, - &mut eviction_state, - ) - .await; - } + // Evict entries if there are any expired entries in the hierarchical + // timer wheels. + if timer_wheel.is_enabled() { + self.evict_expired_entries_using_timers( + &mut timer_wheel, + &mut deqs, + &mut eviction_state, + ) + .await; + } - // TODO: When run_pending_tasks was called explicitly, do not stop evicting - // at the batch size. - if self.has_expiry() || self.has_valid_after() { - self.evict_expired_entries_using_deqs( - &mut deqs, - &mut timer_wheel, - batch_size::EVICTION_BATCH_SIZE, - &mut eviction_state, - ) - .await; - } + // Evict entries if there are any expired entries in the write order or + // access order deques. + if self.has_expiry() || self.has_valid_after() { + self.evict_expired_entries_using_deqs( + &mut deqs, + &mut timer_wheel, + EVICTION_BATCH_SIZE, + &mut eviction_state, + ) + .await; + } - // TODO: When run_pending_tasks was called explicitly, do not stop - // invalidating at the batch size. - if let Some(invalidator) = &self.invalidator { - if !invalidator.is_empty() { - self.invalidate_entries( - invalidator, + // Evict entries if there are any invalidation predicates set by the + // `invalidate_entries_if` method. + if let Some(invalidator) = &self.invalidator { + if !invalidator.is_empty() { + self.invalidate_entries( + invalidator, + &mut deqs, + &mut timer_wheel, + INVALIDATION_BATCH_SIZE, + &mut eviction_state, + ) + .await; + } + } + + // Evict if this cache has more entries than its capacity. + let weights_to_evict = self.weights_to_evict(&eviction_state.counters); + if weights_to_evict > 0 { + self.evict_lru_entries( &mut deqs, &mut timer_wheel, - batch_size::INVALIDATION_BATCH_SIZE, + EVICTION_BATCH_SIZE, + weights_to_evict, &mut eviction_state, ) .await; } - } - // TODO: When run_pending_tasks was called explicitly, do not stop evicting - // at the batch size. + // Check whether to continue this loop or not. - // Evict if this cache has more entries than its capacity. - let weights_to_evict = self.weights_to_evict(&eviction_state.counters); - if weights_to_evict > 0 { - self.evict_lru_entries( - &mut deqs, - &mut timer_wheel, - batch_size::EVICTION_BATCH_SIZE, - weights_to_evict, - &mut eviction_state, - ) - .await; + let should_process_logs = calls <= max_repeats + && (self.read_op_ch.len() >= READ_LOG_FLUSH_POINT + || self.write_op_ch.len() >= WRITE_LOG_FLUSH_POINT); + + if !should_process_logs && !eviction_state.more_entries_to_evict { + break; + } + + if let (Some(to), Some(started)) = (timeout, started_at) { + let elapsed = self + .current_time_from_expiration_clock() + .checked_duration_since(started) + .expect("Arithmetic overflow occurred on calculating the elapse time"); + if elapsed >= to { + break; + } + } } debug_assert_eq!(self.entry_count.load(), current_ec); @@ -1543,6 +1557,8 @@ where // Ensure this lock is held until here. drop(deqs); + + eviction_state.more_entries_to_evict } } @@ -1753,7 +1769,7 @@ where if eviction_state.is_notifier_enabled() { let key = Arc::clone(&kh.key); eviction_state - .add_removed_entry(key, &entry, RemovalCause::Size) + .notify_entry_removal(key, &entry, RemovalCause::Size) .await; } } @@ -1800,7 +1816,7 @@ where ) { if eviction_state.is_notifier_enabled() { eviction_state - .add_removed_entry(vic_key, &vic_entry, RemovalCause::Size) + .notify_entry_removal(vic_key, &vic_entry, RemovalCause::Size) .await; } // And then remove the victim from the deques. @@ -1856,7 +1872,7 @@ where entry.entry_info().set_policy_gen(gen); if eviction_state.is_notifier_enabled() { eviction_state - .add_removed_entry(key, &entry, RemovalCause::Size) + .notify_entry_removal(key, &entry, RemovalCause::Size) .await; } } @@ -2108,7 +2124,7 @@ where // Process each expired key. // // If it is dirty or `cache.remove_if` returns `None`, we will skip it as it - // has been read, updated or invalidated by other thread. + // may have been read, updated or invalidated by other thread. // // - The timer node should have been unset in the current `ValueEntry` as // described above. @@ -2139,7 +2155,7 @@ where if let Some(entry) = maybe_entry { if eviction_state.is_notifier_enabled() { eviction_state - .add_removed_entry(key, &entry, RemovalCause::Expired) + .notify_entry_removal(key, &entry, RemovalCause::Expired) .await; } Self::handle_remove_without_timer_wheel( @@ -2149,7 +2165,7 @@ where &mut eviction_state.counters, ); } else { - // Skip this entry as the key might have been read, updated or + // Skip this entry as the key may have been read, updated or // invalidated by other thread. } } @@ -2199,6 +2215,7 @@ where let tti = &self.expiration_policy.time_to_idle(); let va = &self.valid_after(); let deq_name = cache_region.name(); + let mut more_to_evict = true; for _ in 0..batch_size { let maybe_key_hash_ts = deqs.select_mut(cache_region).0.peek_front().map(|node| { @@ -2216,7 +2233,10 @@ where let cause = match is_entry_expired_ao_or_invalid(tti, va, ts, now) { (true, _) => RemovalCause::Expired, (false, true) => RemovalCause::Explicit, - (false, false) => break, + (false, false) => { + more_to_evict = false; + break; + } }; (key, hash, cause) } @@ -2228,7 +2248,10 @@ where self.skip_updated_entry_ao(&key, hash, deq_name, ao_deq, wo_deq); continue; } - None => break, + None => { + more_to_evict = false; + break; + } }; // Lock the key for removal if blocking removal notification is enabled. @@ -2251,7 +2274,9 @@ where if let Some(entry) = maybe_entry { if eviction_state.is_notifier_enabled() { - eviction_state.add_removed_entry(key, &entry, cause).await; + eviction_state + .notify_entry_removal(key, &entry, cause) + .await; } let (ao_deq, wo_deq) = deqs.select_mut(cache_region); Self::handle_remove_with_deques( @@ -2267,6 +2292,10 @@ where self.skip_updated_entry_ao(&key, hash, deq_name, ao_deq, wo_deq); } } + + if more_to_evict { + eviction_state.more_entries_to_evict = true; + } } #[inline] @@ -2323,6 +2352,7 @@ where { let ttl = &self.expiration_policy.time_to_live(); let va = &self.valid_after(); + let mut more_to_evict = true; for _ in 0..batch_size { let maybe_key_hash_ts = deqs.write_order.peek_front().map(|node| { @@ -2340,7 +2370,10 @@ where let cause = match is_entry_expired_wo_or_invalid(ttl, va, ts, now) { (true, _) => RemovalCause::Expired, (false, true) => RemovalCause::Explicit, - (false, false) => break, + (false, false) => { + more_to_evict = false; + break; + } }; (key, hash, cause) } @@ -2351,7 +2384,10 @@ where self.skip_updated_entry_wo(&key, hash, deqs); continue; } - None => break, + None => { + more_to_evict = false; + break; + } }; // Lock the key for removal if blocking removal notification is enabled. @@ -2370,13 +2406,19 @@ where if let Some(entry) = maybe_entry { if eviction_state.is_notifier_enabled() { - eviction_state.add_removed_entry(key, &entry, cause).await; + eviction_state + .notify_entry_removal(key, &entry, cause) + .await; } Self::handle_remove(deqs, timer_wheel, entry, None, &mut eviction_state.counters); } else { self.skip_updated_entry_wo(&key, hash, deqs); } } + + if more_to_evict { + eviction_state.more_entries_to_evict = true; + } } async fn invalidate_entries( @@ -2398,7 +2440,7 @@ where return; } - let mut candidates = Vec::with_capacity(batch_size); + let mut candidates = Vec::default(); let mut len = 0; let has_next; { @@ -2437,6 +2479,9 @@ where if is_done { deqs.write_order.reset_cursor(); } + if !invalidator.is_empty() { + eviction_state.more_entries_to_evict = true; + } } async fn evict_lru_entries( @@ -2452,9 +2497,11 @@ where const CACHE_REGION: CacheRegion = CacheRegion::MainProbation; let deq_name = CACHE_REGION.name(); let mut evicted = 0u64; + let mut more_to_evict = true; for _ in 0..batch_size { if evicted >= weights_to_evict { + more_to_evict = false; break; } @@ -2482,7 +2529,10 @@ where self.skip_updated_entry_ao(&key, hash, deq_name, ao_deq, wo_deq); continue; } - None => break, + None => { + more_to_evict = false; + break; + } }; // Lock the key for removal if blocking removal notification is enabled. @@ -2508,7 +2558,7 @@ where if let Some(entry) = maybe_entry { if eviction_state.is_notifier_enabled() { eviction_state - .add_removed_entry(key, &entry, RemovalCause::Size) + .notify_entry_removal(key, &entry, RemovalCause::Size) .await; } let weight = entry.policy_weight(); @@ -2527,6 +2577,10 @@ where self.skip_updated_entry_ao(&key, hash, deq_name, ao_deq, wo_deq); } } + + if more_to_evict { + eviction_state.more_entries_to_evict = true; + } } } @@ -2895,19 +2949,19 @@ mod tests { ($cache:ident, $key:ident, $hash:ident, $mock:ident, $duration_secs:expr) => { // Increment the time. $mock.increment(Duration::from_millis($duration_secs * 1000 - 1)); - $cache.inner.do_run_pending_tasks(1).await; + $cache.inner.do_run_pending_tasks(1, None).await; assert!($cache.contains_key_with_hash(&$key, $hash)); assert_eq!($cache.entry_count(), 1); // Increment the time by 1ms (3). The entry should be expired. $mock.increment(Duration::from_millis(1)); - $cache.inner.do_run_pending_tasks(1).await; + $cache.inner.do_run_pending_tasks(1, None).await; assert!(!$cache.contains_key_with_hash(&$key, $hash)); // Increment the time again to ensure the entry has been evicted from the // cache. $mock.increment(Duration::from_secs(1)); - $cache.inner.do_run_pending_tasks(1).await; + $cache.inner.do_run_pending_tasks(1, None).await; assert_eq!($cache.entry_count(), 0); }; } @@ -3191,7 +3245,7 @@ mod tests { insert(&cache, key, hash, value).await; // Run a sync to register the entry to the internal data structures including // the timer wheel. - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; assert_eq!(cache.entry_count(), 1); assert_expiry!(cache, key, hash, mock, 1); @@ -3213,12 +3267,12 @@ mod tests { ExpiryExpectation::after_create(line!(), key, value, current_time(&cache), None); let inserted_at = current_time(&cache); insert(&cache, key, hash, value).await; - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(1)); - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; assert!(cache.contains_key_with_hash(&key, hash)); // Read the entry (2). @@ -3238,7 +3292,7 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; assert_expiry!(cache, key, hash, mock, 3); @@ -3260,12 +3314,12 @@ mod tests { ExpiryExpectation::after_create(line!(), key, value, current_time(&cache), None); let inserted_at = current_time(&cache); insert(&cache, key, hash, value).await; - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(1)); - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; assert!(cache.contains_key_with_hash(&key, hash)); // Read the entry (2). @@ -3285,11 +3339,11 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; // Increment the time. mock.increment(Duration::from_secs(2)); - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3304,7 +3358,7 @@ mod tests { Some(3), ); insert(&cache, key, hash, value).await; - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; assert_eq!(cache.entry_count(), 1); assert_expiry!(cache, key, hash, mock, 3); @@ -3327,12 +3381,12 @@ mod tests { ExpiryExpectation::after_create(line!(), key, value, current_time(&cache), None); let inserted_at = current_time(&cache); insert(&cache, key, hash, value).await; - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(1)); - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3353,11 +3407,11 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; // Increment the time. mock.increment(Duration::from_secs(2)); - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3372,7 +3426,7 @@ mod tests { None, ); insert(&cache, key, hash, value).await; - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; assert_eq!(cache.entry_count(), 1); assert_expiry!(cache, key, hash, mock, 7); @@ -3394,12 +3448,12 @@ mod tests { ExpiryExpectation::after_create(line!(), key, value, current_time(&cache), Some(8)); let inserted_at = current_time(&cache); insert(&cache, key, hash, value).await; - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(5)); - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3420,7 +3474,7 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; assert_expiry!(cache, key, hash, mock, 7); @@ -3442,12 +3496,12 @@ mod tests { ExpiryExpectation::after_create(line!(), key, value, current_time(&cache), Some(8)); let inserted_at = current_time(&cache); insert(&cache, key, hash, value).await; - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(5)); - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3468,11 +3522,11 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; // Increment the time. mock.increment(Duration::from_secs(6)); - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3493,7 +3547,7 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; assert_expiry!(cache, key, hash, mock, 5); @@ -3514,12 +3568,12 @@ mod tests { *expectation.lock().unwrap() = ExpiryExpectation::after_create(line!(), key, value, current_time(&cache), Some(9)); insert(&cache, key, hash, value).await; - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(6)); - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3535,12 +3589,12 @@ mod tests { ); let updated_at = current_time(&cache); insert(&cache, key, hash, value).await; - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(6)); - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3561,11 +3615,11 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; // Increment the time. mock.increment(Duration::from_secs(6)); - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3586,7 +3640,7 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.do_run_pending_tasks(1).await; + cache.inner.do_run_pending_tasks(1, None).await; assert_expiry!(cache, key, hash, mock, 4); } diff --git a/src/future/housekeeper.rs b/src/future/housekeeper.rs index 60fc9111..201ad168 100644 --- a/src/future/housekeeper.rs +++ b/src/future/housekeeper.rs @@ -2,8 +2,8 @@ use crate::common::{ concurrent::{ atomic_time::AtomicInstant, constants::{ - MAX_SYNC_REPEATS, PERIODICAL_SYNC_INITIAL_DELAY_MILLIS, READ_LOG_FLUSH_POINT, - WRITE_LOG_FLUSH_POINT, + DEFAULT_RUN_PENDING_TASKS_TIMEOUT_MILLIS, MAX_SYNC_REPEATS, + PERIODICAL_SYNC_INITIAL_DELAY_MILLIS, READ_LOG_FLUSH_POINT, WRITE_LOG_FLUSH_POINT, }, }, time::{CheckedTimeOps, Instant}, @@ -26,7 +26,8 @@ use futures_util::future::{BoxFuture, Shared}; #[async_trait] pub(crate) trait InnerSync { - async fn run_pending_tasks(&self, max_sync_repeats: usize); + /// Runs the pending tasks. Returns `true` if there are more entries to evict. + async fn run_pending_tasks(&self, max_sync_repeats: usize, timeout: Option) -> bool; /// Notifies all the async tasks waiting in `BaseCache::schedule_write_op` method /// for the write op channel to have enough room. @@ -37,8 +38,21 @@ pub(crate) trait InnerSync { pub(crate) struct Housekeeper { /// A shared `Future` of the maintenance task that is currently being resolved. - current_task: Mutex>>>, + current_task: Mutex>>>, run_after: AtomicInstant, + /// A flag to indicate if the last `run_pending_tasks` call left more entries to + /// evict. + /// + /// Used only when the eviction listener closure is set for this cache instance + /// because, if not, `run_pending_tasks` will never leave more entries to evict. + more_entries_to_evict: Option, + /// The timeout duration for the `run_pending_tasks` method. This is a safe-guard + /// to prevent cache read/write operations (that may call `run_pending_tasks` + /// internally) from being blocked for a long time when the user wrote a slow + /// eviction listener closure. + /// + /// Used only when the eviction listener closure is set for this cache instance. + maintenance_task_timeout: Option, auto_run_enabled: AtomicBool, #[cfg(test)] pub(crate) start_count: AtomicUsize, @@ -46,11 +60,24 @@ pub(crate) struct Housekeeper { pub(crate) complete_count: AtomicUsize, } -impl Default for Housekeeper { - fn default() -> Self { +impl Housekeeper { + pub(crate) fn new(is_eviction_listener_enabled: bool) -> Self { + let (more_entries_to_evict, maintenance_task_timeout) = if is_eviction_listener_enabled { + ( + Some(AtomicBool::new(false)), + Some(Duration::from_millis( + DEFAULT_RUN_PENDING_TASKS_TIMEOUT_MILLIS, + )), + ) + } else { + (None, None) + }; + Self { current_task: Mutex::default(), run_after: AtomicInstant::new(Self::sync_after(Instant::now())), + more_entries_to_evict, + maintenance_task_timeout, auto_run_enabled: AtomicBool::new(true), #[cfg(test)] start_count: Default::default(), @@ -58,15 +85,29 @@ impl Default for Housekeeper { complete_count: Default::default(), } } -} -impl Housekeeper { pub(crate) fn should_apply_reads(&self, ch_len: usize, now: Instant) -> bool { - self.should_apply(ch_len, READ_LOG_FLUSH_POINT / 8, now) + self.more_entries_to_evict() || self.should_apply(ch_len, READ_LOG_FLUSH_POINT, now) + // self.should_apply(ch_len, READ_LOG_FLUSH_POINT, now) } pub(crate) fn should_apply_writes(&self, ch_len: usize, now: Instant) -> bool { - self.should_apply(ch_len, WRITE_LOG_FLUSH_POINT / 8, now) + self.more_entries_to_evict() || self.should_apply(ch_len, WRITE_LOG_FLUSH_POINT, now) + // self.should_apply(ch_len, WRITE_LOG_FLUSH_POINT, now) + } + + #[inline] + fn more_entries_to_evict(&self) -> bool { + self.more_entries_to_evict + .as_ref() + .map(|v| v.load(Ordering::Acquire)) + .unwrap_or(false) + } + + fn set_more_entries_to_evict(&self, v: bool) { + if let Some(flag) = &self.more_entries_to_evict { + flag.store(v, Ordering::Release); + } } #[inline] @@ -112,24 +153,25 @@ impl Housekeeper { async fn do_run_pending_tasks( &self, cache: Arc, - current_task: &mut Option>>, + current_task: &mut Option>>, ) where T: InnerSync + Send + Sync + 'static, { use futures_util::FutureExt; let now = cache.now(); - + let more_to_evict; // Async Cancellation Safety: Our maintenance task is cancellable as we save // it in the lock. If it is canceled, we will resume it in the next run. if let Some(task) = &*current_task { // This task was cancelled in the previous run due to the enclosing // Future was dropped. Resume the task now by awaiting. - task.clone().await; + more_to_evict = task.clone().await; } else { + let timeout = self.maintenance_task_timeout; // Create a new maintenance task and await it. - let task = async move { cache.run_pending_tasks(MAX_SYNC_REPEATS).await } + let task = async move { cache.run_pending_tasks(MAX_SYNC_REPEATS, timeout).await } .boxed() .shared(); *current_task = Some(task.clone()); @@ -137,13 +179,14 @@ impl Housekeeper { #[cfg(test)] self.start_count.fetch_add(1, Ordering::AcqRel); - task.await; + more_to_evict = task.await; } // If we are here, it means that the maintenance task has been completed. // We can remove it from the lock. *current_task = None; self.run_after.set_instant(Self::sync_after(now)); + self.set_more_entries_to_evict(more_to_evict); #[cfg(test)] self.complete_count.fetch_add(1, Ordering::AcqRel); diff --git a/src/sync_base/base_cache.rs b/src/sync_base/base_cache.rs index b4e2b6a7..b95adf57 100644 --- a/src/sync_base/base_cache.rs +++ b/src/sync_base/base_cache.rs @@ -11,7 +11,8 @@ use crate::{ concurrent::{ atomic_time::AtomicInstant, constants::{ - READ_LOG_FLUSH_POINT, READ_LOG_SIZE, WRITE_LOG_FLUSH_POINT, WRITE_LOG_SIZE, + EVICTION_BATCH_SIZE, INVALIDATION_BATCH_SIZE, READ_LOG_FLUSH_POINT, READ_LOG_SIZE, + WRITE_LOG_FLUSH_POINT, WRITE_LOG_SIZE, }, deques::Deques, entry_info::EntryInfo, @@ -763,7 +764,7 @@ impl<'a, K, V> EvictionState<'a, K, V> { self.notifier.is_some() } - fn add_removed_entry( + fn notify_entry_removal( &mut self, key: Arc, entry: &TrioArc>, @@ -772,10 +773,10 @@ impl<'a, K, V> EvictionState<'a, K, V> { K: Send + Sync + 'static, V: Clone + Send + Sync + 'static, { - debug_assert!(self.is_notifier_enabled()); - if let Some(notifier) = self.notifier { notifier.notify(key, entry.value.clone(), cause); + } else { + panic!("notify_entry_removal is called when the notification is disabled"); } } } @@ -1226,21 +1227,6 @@ where } } -// TODO: Calculate the batch size based on the number of entries in the cache (or an -// estimated number of entries to evict) - -#[cfg(feature = "unstable-debug-counters")] -mod batch_size { - pub(crate) const EVICTION_BATCH_SIZE: usize = 10_000; - pub(crate) const INVALIDATION_BATCH_SIZE: usize = 10_000; -} - -#[cfg(not(feature = "unstable-debug-counters"))] -mod batch_size { - pub(crate) const EVICTION_BATCH_SIZE: usize = 500; - pub(crate) const INVALIDATION_BATCH_SIZE: usize = 500; -} - impl InnerSync for Inner where K: Hash + Eq + Send + Sync + 'static, @@ -1315,7 +1301,7 @@ where self.evict_expired_entries_using_deqs( &mut deqs, &mut timer_wheel, - batch_size::EVICTION_BATCH_SIZE, + EVICTION_BATCH_SIZE, &mut eviction_state, ); } @@ -1328,7 +1314,7 @@ where invalidator, &mut deqs, &mut timer_wheel, - batch_size::INVALIDATION_BATCH_SIZE, + INVALIDATION_BATCH_SIZE, &mut eviction_state, ); } @@ -1343,7 +1329,7 @@ where self.evict_lru_entries( &mut deqs, &mut timer_wheel, - batch_size::EVICTION_BATCH_SIZE, + EVICTION_BATCH_SIZE, weights_to_evict, &mut eviction_state, ); @@ -1553,7 +1539,7 @@ where if let Some(entry) = removed { if eviction_state.is_notifier_enabled() { let key = Arc::clone(&kh.key); - eviction_state.add_removed_entry(key, &entry, RemovalCause::Size); + eviction_state.notify_entry_removal(key, &entry, RemovalCause::Size); } } entry.entry_info().set_policy_gen(gen); @@ -1594,7 +1580,7 @@ where |k, v| (k.clone(), v.clone()), ) { if eviction_state.is_notifier_enabled() { - eviction_state.add_removed_entry( + eviction_state.notify_entry_removal( vic_key, &vic_entry, RemovalCause::Size, @@ -1649,7 +1635,7 @@ where if let Some(entry) = removed { entry.entry_info().set_policy_gen(gen); if eviction_state.is_notifier_enabled() { - eviction_state.add_removed_entry(key, &entry, RemovalCause::Size); + eviction_state.notify_entry_removal(key, &entry, RemovalCause::Size); } } } @@ -1886,7 +1872,7 @@ where // timer node pointer in the `ValueEntry`, so we do not have to do it // here. // 2. If an entry is dirty or `cache.remove_if` returns `None`, we will skip - // it as it has been read, updated or invalidated by other thread. + // it as it may have been read, updated or invalidated by other thread. // - The timer node should have been unset in the current `ValueEntry` as // described above. // - When necessary, a new timer node will be recreated for the current or @@ -1922,7 +1908,7 @@ where if let Some(entry) = maybe_entry { if eviction_state.is_notifier_enabled() { let key = Arc::clone(key); - eviction_state.add_removed_entry(key, &entry, RemovalCause::Expired); + eviction_state.notify_entry_removal(key, &entry, RemovalCause::Expired); } Self::handle_remove_without_timer_wheel( deqs, @@ -1931,7 +1917,7 @@ where &mut eviction_state.counters, ); } else { - // Skip this entry as the key might have been read, updated or + // Skip this entry as the key may have been read, updated or // invalidated by other thread. } } @@ -2026,7 +2012,7 @@ where if let Some(entry) = maybe_entry { if eviction_state.is_notifier_enabled() { - eviction_state.add_removed_entry(key, &entry, cause); + eviction_state.notify_entry_removal(key, &entry, cause); } Self::handle_remove_with_deques( deq_name, @@ -2139,7 +2125,7 @@ where if let Some(entry) = maybe_entry { if eviction_state.is_notifier_enabled() { - eviction_state.add_removed_entry(key, &entry, cause); + eviction_state.notify_entry_removal(key, &entry, cause); } Self::handle_remove(deqs, timer_wheel, entry, None, &mut eviction_state.counters); } else { @@ -2267,7 +2253,7 @@ where if let Some(entry) = maybe_entry { if eviction_state.is_notifier_enabled() { - eviction_state.add_removed_entry(key, &entry, RemovalCause::Size); + eviction_state.notify_entry_removal(key, &entry, RemovalCause::Size); } let weight = entry.policy_weight(); Self::handle_remove_with_deques( From 3b9adc395bc756747d8b3217d4fccdefd65899b3 Mon Sep 17 00:00:00 2001 From: Tatsuya Kawano Date: Sun, 14 Apr 2024 11:42:18 +0800 Subject: [PATCH 2/9] Make `run_pending_tasks` to evict more entries from the cache at once Apply the same change of `future::Cache` to `sync` caches. --- src/common/concurrent/housekeeper.rs | 62 ++++++- src/future/housekeeper.rs | 2 - src/sync_base/base_cache.rs | 236 +++++++++++++++++---------- 3 files changed, 203 insertions(+), 97 deletions(-) diff --git a/src/common/concurrent/housekeeper.rs b/src/common/concurrent/housekeeper.rs index 185eff52..2ded85f7 100644 --- a/src/common/concurrent/housekeeper.rs +++ b/src/common/concurrent/housekeeper.rs @@ -1,4 +1,7 @@ -use super::constants::{MAX_SYNC_REPEATS, PERIODICAL_SYNC_INITIAL_DELAY_MILLIS}; +use super::constants::{ + DEFAULT_RUN_PENDING_TASKS_TIMEOUT_MILLIS, MAX_SYNC_REPEATS, + PERIODICAL_SYNC_INITIAL_DELAY_MILLIS, +}; use super::{ atomic_time::AtomicInstant, @@ -13,7 +16,8 @@ use std::{ }; pub(crate) trait InnerSync { - fn run_pending_tasks(&self, max_sync_repeats: usize); + /// Runs the pending tasks. Returns `true` if there are more entries to evict. + fn run_pending_tasks(&self, max_sync_repeats: usize, timeout: Option) -> bool; fn now(&self) -> Instant; } @@ -21,26 +25,64 @@ pub(crate) trait InnerSync { pub(crate) struct Housekeeper { run_lock: Mutex<()>, run_after: AtomicInstant, + /// A flag to indicate if the last `run_pending_tasks` call left more entries to + /// evict. + /// + /// Used only when the eviction listener closure is set for this cache instance + /// because, if not, `run_pending_tasks` will never leave more entries to evict. + more_entries_to_evict: Option, + /// The timeout duration for the `run_pending_tasks` method. This is a safe-guard + /// to prevent cache read/write operations (that may call `run_pending_tasks` + /// internally) from being blocked for a long time when the user wrote a slow + /// eviction listener closure. + /// + /// Used only when the eviction listener closure is set for this cache instance. + maintenance_task_timeout: Option, auto_run_enabled: AtomicBool, } -impl Default for Housekeeper { - fn default() -> Self { +impl Housekeeper { + pub(crate) fn new(is_eviction_listener_enabled: bool) -> Self { + let (more_entries_to_evict, maintenance_task_timeout) = if is_eviction_listener_enabled { + ( + Some(AtomicBool::new(false)), + Some(Duration::from_millis( + DEFAULT_RUN_PENDING_TASKS_TIMEOUT_MILLIS, + )), + ) + } else { + (None, None) + }; + Self { run_lock: Mutex::default(), run_after: AtomicInstant::new(Self::sync_after(Instant::now())), + more_entries_to_evict, + maintenance_task_timeout, auto_run_enabled: AtomicBool::new(true), } } -} -impl Housekeeper { pub(crate) fn should_apply_reads(&self, ch_len: usize, now: Instant) -> bool { - self.should_apply(ch_len, READ_LOG_FLUSH_POINT, now) + self.more_entries_to_evict() || self.should_apply(ch_len, READ_LOG_FLUSH_POINT, now) } pub(crate) fn should_apply_writes(&self, ch_len: usize, now: Instant) -> bool { - self.should_apply(ch_len, WRITE_LOG_FLUSH_POINT, now) + self.more_entries_to_evict() || self.should_apply(ch_len, WRITE_LOG_FLUSH_POINT, now) + } + + #[inline] + fn more_entries_to_evict(&self) -> bool { + self.more_entries_to_evict + .as_ref() + .map(|v| v.load(Ordering::Acquire)) + .unwrap_or(false) + } + + fn set_more_entries_to_evict(&self, v: bool) { + if let Some(flag) = &self.more_entries_to_evict { + flag.store(v, Ordering::Release); + } } #[inline] @@ -66,7 +108,9 @@ impl Housekeeper { fn do_run_pending_tasks(&self, cache: &T, _lock: MutexGuard<'_, ()>) { let now = cache.now(); self.run_after.set_instant(Self::sync_after(now)); - cache.run_pending_tasks(MAX_SYNC_REPEATS); + let timeout = self.maintenance_task_timeout; + let more_to_evict = cache.run_pending_tasks(MAX_SYNC_REPEATS, timeout); + self.set_more_entries_to_evict(more_to_evict); } fn sync_after(now: Instant) -> Instant { diff --git a/src/future/housekeeper.rs b/src/future/housekeeper.rs index 201ad168..2fd81e44 100644 --- a/src/future/housekeeper.rs +++ b/src/future/housekeeper.rs @@ -88,12 +88,10 @@ impl Housekeeper { pub(crate) fn should_apply_reads(&self, ch_len: usize, now: Instant) -> bool { self.more_entries_to_evict() || self.should_apply(ch_len, READ_LOG_FLUSH_POINT, now) - // self.should_apply(ch_len, READ_LOG_FLUSH_POINT, now) } pub(crate) fn should_apply_writes(&self, ch_len: usize, now: Instant) -> bool { self.more_entries_to_evict() || self.should_apply(ch_len, WRITE_LOG_FLUSH_POINT, now) - // self.should_apply(ch_len, WRITE_LOG_FLUSH_POINT, now) } #[inline] diff --git a/src/sync_base/base_cache.rs b/src/sync_base/base_cache.rs index b95adf57..61d557cc 100644 --- a/src/sync_base/base_cache.rs +++ b/src/sync_base/base_cache.rs @@ -153,6 +153,7 @@ where } else { (READ_LOG_SIZE, WRITE_LOG_SIZE) }; + let is_eviction_listener_enabled = eviction_listener.is_some(); let (r_snd, r_rcv) = crossbeam_channel::bounded(r_size); let (w_snd, w_rcv) = crossbeam_channel::bounded(w_size); @@ -175,7 +176,7 @@ where inner, read_op_ch: r_snd, write_op_ch: w_snd, - housekeeper: Some(Arc::new(Housekeeper::default())), + housekeeper: Some(Arc::new(Housekeeper::new(is_eviction_listener_enabled))), } } @@ -746,6 +747,7 @@ where struct EvictionState<'a, K, V> { counters: EvictionCounters, notifier: Option<&'a RemovalNotifier>, + more_entries_to_evict: bool, } impl<'a, K, V> EvictionState<'a, K, V> { @@ -757,6 +759,7 @@ impl<'a, K, V> EvictionState<'a, K, V> { Self { counters: EvictionCounters::new(entry_count, weighted_size), notifier, + more_entries_to_evict: false, } } @@ -1233,8 +1236,8 @@ where V: Clone + Send + Sync + 'static, S: BuildHasher + Clone + Send + Sync + 'static, { - fn run_pending_tasks(&self, max_repeats: usize) { - self.do_run_pending_tasks(max_repeats); + fn run_pending_tasks(&self, max_repeats: usize, timeout: Option) -> bool { + self.do_run_pending_tasks(max_repeats, timeout) } fn now(&self) -> Instant { @@ -1248,24 +1251,29 @@ where V: Clone + Send + Sync + 'static, S: BuildHasher + Clone + Send + Sync + 'static, { - fn do_run_pending_tasks(&self, max_repeats: usize) { + fn do_run_pending_tasks(&self, max_repeats: usize, timeout: Option) -> bool { if self.max_capacity == Some(0) { - return; + return false; } // Acquire some locks. let mut deqs = self.deques.lock(); let mut timer_wheel = self.timer_wheel.lock(); + let started_at = if timeout.is_some() { + Some(self.current_time_from_expiration_clock()) + } else { + None + }; let mut calls = 0; let current_ec = self.entry_count.load(); let current_ws = self.weighted_size.load(); let mut eviction_state = EvictionState::new(current_ec, current_ws, self.removal_notifier.as_ref()); - let mut should_process_logs = true; + loop { + calls += 1; - while should_process_logs && calls <= max_repeats { let r_len = self.read_op_ch.len(); if r_len > 0 { self.apply_reads(&mut deqs, &mut timer_wheel, r_len); @@ -1282,57 +1290,77 @@ where self.enable_frequency_sketch(&eviction_state.counters); } - calls += 1; - should_process_logs = self.read_op_ch.len() >= READ_LOG_FLUSH_POINT - || self.write_op_ch.len() >= WRITE_LOG_FLUSH_POINT; - } + // Set this flag to `false`. The `evict_*` and `invalidate_*` methods + // below may set it to `true` if there are more entries to evict in next + // loop. + eviction_state.more_entries_to_evict = false; - if timer_wheel.is_enabled() { - self.evict_expired_entries_using_timers( - &mut timer_wheel, - &mut deqs, - &mut eviction_state, - ); - } + // Evict entries if there are any expired entries in the hierarchical + // timer wheels. + if timer_wheel.is_enabled() { + self.evict_expired_entries_using_timers( + &mut timer_wheel, + &mut deqs, + &mut eviction_state, + ); + } - // TODO: When run_pending_tasks was called explicitly, do not stop evicting - // at the batch size. - if self.has_expiry() || self.has_valid_after() { - self.evict_expired_entries_using_deqs( - &mut deqs, - &mut timer_wheel, - EVICTION_BATCH_SIZE, - &mut eviction_state, - ); - } + // Evict entries if there are any expired entries in the write order or + // access order deques. + if self.has_expiry() || self.has_valid_after() { + self.evict_expired_entries_using_deqs( + &mut deqs, + &mut timer_wheel, + EVICTION_BATCH_SIZE, + &mut eviction_state, + ); + } + + // Evict entries if there are any invalidation predicates set by the + // `invalidate_entries_if` method. + if let Some(invalidator) = &self.invalidator { + if !invalidator.is_empty() { + self.invalidate_entries( + invalidator, + &mut deqs, + &mut timer_wheel, + INVALIDATION_BATCH_SIZE, + &mut eviction_state, + ); + } + } - // TODO: When run_pending_tasks was called explicitly, do not stop - // invalidating at the batch size. - if let Some(invalidator) = &self.invalidator { - if !invalidator.is_empty() { - self.invalidate_entries( - invalidator, + // Evict if this cache has more entries than its capacity. + let weights_to_evict = self.weights_to_evict(&eviction_state.counters); + if weights_to_evict > 0 { + self.evict_lru_entries( &mut deqs, &mut timer_wheel, - INVALIDATION_BATCH_SIZE, + EVICTION_BATCH_SIZE, + weights_to_evict, &mut eviction_state, ); } - } - // TODO: When run_pending_tasks was called explicitly, do not stop evicting - // at the batch size. + // Check whether to continue this loop or not. - // Evict if this cache has more entries than its capacity. - let weights_to_evict = self.weights_to_evict(&eviction_state.counters); - if weights_to_evict > 0 { - self.evict_lru_entries( - &mut deqs, - &mut timer_wheel, - EVICTION_BATCH_SIZE, - weights_to_evict, - &mut eviction_state, - ); + let should_process_logs = calls <= max_repeats + && (self.read_op_ch.len() >= READ_LOG_FLUSH_POINT + || self.write_op_ch.len() >= WRITE_LOG_FLUSH_POINT); + + if !should_process_logs && !eviction_state.more_entries_to_evict { + break; + } + + if let (Some(to), Some(started)) = (timeout, started_at) { + let elapsed = self + .current_time_from_expiration_clock() + .checked_duration_since(started) + .expect("Arithmetic overflow occurred on calculating the elapse time"); + if elapsed >= to { + break; + } + } } debug_assert_eq!(self.entry_count.load(), current_ec); @@ -1345,6 +1373,8 @@ where // Ensure the deqs lock is held until here. drop(deqs); + + eviction_state.more_entries_to_evict } } @@ -1965,6 +1995,7 @@ where let va = &self.valid_after(); let deq_name = cache_region.name(); let (ao_deq, wo_deq) = deqs.select_mut(cache_region); + let mut more_to_evict = true; for _ in 0..batch_size { let maybe_key_hash_ts = ao_deq.peek_front().map(|node| { @@ -1982,7 +2013,10 @@ where let cause = match is_entry_expired_ao_or_invalid(tti, va, ts, now) { (true, _) => RemovalCause::Expired, (false, true) => RemovalCause::Explicit, - (false, false) => break, + (false, false) => { + more_to_evict = false; + break; + } }; (key, hash, cause) } @@ -1993,7 +2027,10 @@ where self.skip_updated_entry_ao(&key, hash, deq_name, ao_deq, wo_deq); continue; } - None => break, + None => { + more_to_evict = false; + break; + } }; // Lock the key for removal if blocking removal notification is enabled. @@ -2026,6 +2063,10 @@ where self.skip_updated_entry_ao(&key, hash, deq_name, ao_deq, wo_deq); } } + + if more_to_evict { + eviction_state.more_entries_to_evict = true; + } } #[inline] @@ -2082,6 +2123,7 @@ where { let ttl = &self.expiration_policy.time_to_live(); let va = &self.valid_after(); + let mut more_to_evict = true; for _ in 0..batch_size { let maybe_key_hash_ts = deqs.write_order.peek_front().map(|node| { @@ -2099,7 +2141,10 @@ where let cause = match is_entry_expired_wo_or_invalid(ttl, va, ts, now) { (true, _) => RemovalCause::Expired, (false, true) => RemovalCause::Explicit, - (false, false) => break, + (false, false) => { + more_to_evict = false; + break; + } }; (key, hash, cause) } @@ -2110,7 +2155,10 @@ where self.skip_updated_entry_wo(&key, hash, deqs); continue; } - None => break, + None => { + more_to_evict = false; + break; + } }; // Lock the key for removal if blocking removal notification is enabled. @@ -2132,6 +2180,10 @@ where self.skip_updated_entry_wo(&key, hash, deqs); } } + + if more_to_evict { + eviction_state.more_entries_to_evict = true; + } } fn invalidate_entries( @@ -2153,7 +2205,7 @@ where return; } - let mut candidates = Vec::with_capacity(batch_size); + let mut candidates = Vec::new(); let mut len = 0; let has_next; { @@ -2191,6 +2243,9 @@ where if is_done { deqs.write_order.reset_cursor(); } + if !invalidator.is_empty() { + eviction_state.more_entries_to_evict = true; + } } fn evict_lru_entries( @@ -2207,9 +2262,11 @@ where let deq_name = CACHE_REGION.name(); let (ao_deq, wo_deq) = deqs.select_mut(CACHE_REGION); let mut evicted = 0u64; + let mut more_to_evict = true; for _ in 0..batch_size { if evicted >= weights_to_evict { + more_to_evict = false; break; } @@ -2232,7 +2289,10 @@ where self.skip_updated_entry_ao(&key, hash, deq_name, ao_deq, wo_deq); continue; } - None => break, + None => { + more_to_evict = false; + break; + } }; // Lock the key for removal if blocking removal notification is enabled. @@ -2269,6 +2329,10 @@ where self.skip_updated_entry_ao(&key, hash, deq_name, ao_deq, wo_deq); } } + + if more_to_evict { + eviction_state.more_entries_to_evict = true; + } } } @@ -2607,19 +2671,19 @@ mod tests { ($cache:ident, $key:ident, $hash:ident, $mock:ident, $duration_secs:expr) => { // Increment the time. $mock.increment(Duration::from_millis($duration_secs * 1000 - 1)); - $cache.inner.run_pending_tasks(1); + $cache.inner.run_pending_tasks(1, None); assert!($cache.contains_key_with_hash(&$key, $hash)); assert_eq!($cache.entry_count(), 1); // Increment the time by 1ms (3). The entry should be expired. $mock.increment(Duration::from_millis(1)); - $cache.inner.run_pending_tasks(1); + $cache.inner.run_pending_tasks(1, None); assert!(!$cache.contains_key_with_hash(&$key, $hash)); // Increment the time again to ensure the entry has been evicted from the // cache. $mock.increment(Duration::from_secs(1)); - $cache.inner.run_pending_tasks(1); + $cache.inner.run_pending_tasks(1, None); assert_eq!($cache.entry_count(), 0); }; } @@ -2903,7 +2967,7 @@ mod tests { insert(&cache, key, hash, value); // Run a sync to register the entry to the internal data structures including // the timer wheel. - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); assert_eq!(cache.entry_count(), 1); assert_expiry!(cache, key, hash, mock, 1); @@ -2925,12 +2989,12 @@ mod tests { ExpiryExpectation::after_create(line!(), key, value, current_time(&cache), None); let inserted_at = current_time(&cache); insert(&cache, key, hash, value); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(1)); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); assert!(cache.contains_key_with_hash(&key, hash)); // Read the entry (2). @@ -2949,7 +3013,7 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); assert_expiry!(cache, key, hash, mock, 3); @@ -2971,12 +3035,12 @@ mod tests { ExpiryExpectation::after_create(line!(), key, value, current_time(&cache), None); let inserted_at = current_time(&cache); insert(&cache, key, hash, value); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(1)); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); assert!(cache.contains_key_with_hash(&key, hash)); // Read the entry (2). @@ -2995,11 +3059,11 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); // Increment the time. mock.increment(Duration::from_secs(2)); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3014,7 +3078,7 @@ mod tests { Some(3), ); insert(&cache, key, hash, value); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); assert_eq!(cache.entry_count(), 1); assert_expiry!(cache, key, hash, mock, 3); @@ -3037,12 +3101,12 @@ mod tests { ExpiryExpectation::after_create(line!(), key, value, current_time(&cache), None); let inserted_at = current_time(&cache); insert(&cache, key, hash, value); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(1)); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3062,11 +3126,11 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); // Increment the time. mock.increment(Duration::from_secs(2)); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3081,7 +3145,7 @@ mod tests { None, ); insert(&cache, key, hash, value); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); assert_eq!(cache.entry_count(), 1); assert_expiry!(cache, key, hash, mock, 7); @@ -3103,12 +3167,12 @@ mod tests { ExpiryExpectation::after_create(line!(), key, value, current_time(&cache), Some(8)); let inserted_at = current_time(&cache); insert(&cache, key, hash, value); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(5)); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3128,7 +3192,7 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); assert_expiry!(cache, key, hash, mock, 7); @@ -3150,12 +3214,12 @@ mod tests { ExpiryExpectation::after_create(line!(), key, value, current_time(&cache), Some(8)); let inserted_at = current_time(&cache); insert(&cache, key, hash, value); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(5)); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3175,11 +3239,11 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); // Increment the time. mock.increment(Duration::from_secs(6)); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3199,7 +3263,7 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); assert_expiry!(cache, key, hash, mock, 5); @@ -3220,12 +3284,12 @@ mod tests { *expectation.lock().unwrap() = ExpiryExpectation::after_create(line!(), key, value, current_time(&cache), Some(9)); insert(&cache, key, hash, value); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(6)); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3241,12 +3305,12 @@ mod tests { ); let updated_at = current_time(&cache); insert(&cache, key, hash, value); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(6)); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3266,11 +3330,11 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); // Increment the time. mock.increment(Duration::from_secs(6)); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3290,7 +3354,7 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.run_pending_tasks(1); + cache.inner.run_pending_tasks(1, None); assert_expiry!(cache, key, hash, mock, 4); } From 364567eb1d1822674ea4182ad8da6c06d92bac69 Mon Sep 17 00:00:00 2001 From: Tatsuya Kawano Date: Sun, 14 Apr 2024 17:55:23 +0800 Subject: [PATCH 3/9] Make `run_pending_tasks` to evict more entries from the cache at once Add unit tests. --- src/common.rs | 55 +++++++++ src/common/concurrent/constants.rs | 20 ++-- src/common/concurrent/housekeeper.rs | 39 ++++--- src/future/base_cache.rs | 113 ++++++++++-------- src/future/builder.rs | 16 ++- src/future/cache.rs | 162 +++++++++++++++++++++++++- src/future/housekeeper.rs | 39 ++++--- src/sync/builder.rs | 17 ++- src/sync/cache.rs | 164 ++++++++++++++++++++++++++- src/sync/segment.rs | 8 +- src/sync_base/base_cache.rs | 110 ++++++++++-------- 11 files changed, 605 insertions(+), 138 deletions(-) diff --git a/src/common.rs b/src/common.rs index 048963fe..9748e86f 100644 --- a/src/common.rs +++ b/src/common.rs @@ -1,3 +1,5 @@ +use std::time::Duration; + pub(crate) mod builder_utils; pub(crate) mod concurrent; pub(crate) mod deque; @@ -10,6 +12,11 @@ pub(crate) mod timer_wheel; #[cfg(test)] pub(crate) mod test_utils; +use self::concurrent::constants::{ + DEFAULT_EVICTION_BATCH_SIZE, DEFAULT_MAINTENANCE_TASK_TIMEOUT_MILLIS, + DEFAULT_MAX_LOG_SYNC_REPEATS, +}; + // Note: `CacheRegion` cannot have more than four enum variants. This is because // `crate::{sync,unsync}::DeqNodes` uses a `tagptr::TagNonNull, 2>` // pointer, where the 2-bit tag is `CacheRegion`. @@ -56,6 +63,54 @@ impl PartialEq for CacheRegion { } } +#[derive(Clone, Debug)] +pub(crate) struct HousekeeperConfig { + /// The timeout duration for the `run_pending_tasks` method. This is a safe-guard + /// to prevent cache read/write operations (that may call `run_pending_tasks` + /// internally) from being blocked for a long time when the user wrote a slow + /// eviction listener closure. + /// + /// Used only when the eviction listener closure is set for the cache instance. + /// + /// Default: `DEFAULT_MAINTENANCE_TASK_TIMEOUT_MILLIS` + pub(crate) maintenance_task_timeout: Duration, + /// The maximum repeat count for receiving operation logs from the read and write + /// log channels. Default: `MAX_LOG_SYNC_REPEATS`. + pub(crate) max_log_sync_repeats: usize, + /// The batch size of entries to be processed by each internal eviction method. + /// Default: `EVICTION_BATCH_SIZE`. + pub(crate) eviction_batch_size: usize, +} + +impl Default for HousekeeperConfig { + fn default() -> Self { + Self { + maintenance_task_timeout: Duration::from_millis( + DEFAULT_MAINTENANCE_TASK_TIMEOUT_MILLIS, + ), + max_log_sync_repeats: DEFAULT_MAX_LOG_SYNC_REPEATS, + eviction_batch_size: DEFAULT_EVICTION_BATCH_SIZE, + } + } +} + +impl HousekeeperConfig { + #[cfg(test)] + pub(crate) fn new( + maintenance_task_timeout: Option, + max_log_sync_repeats: Option, + eviction_batch_size: Option, + ) -> Self { + Self { + maintenance_task_timeout: maintenance_task_timeout.unwrap_or(Duration::from_millis( + DEFAULT_MAINTENANCE_TASK_TIMEOUT_MILLIS, + )), + max_log_sync_repeats: max_log_sync_repeats.unwrap_or(DEFAULT_MAX_LOG_SYNC_REPEATS), + eviction_batch_size: eviction_batch_size.unwrap_or(DEFAULT_EVICTION_BATCH_SIZE), + } + } +} + // Ensures the value fits in a range of `128u32..=u32::MAX`. pub(crate) fn sketch_capacity(max_capacity: u64) -> u32 { max_capacity.try_into().unwrap_or(u32::MAX).max(128) diff --git a/src/common/concurrent/constants.rs b/src/common/concurrent/constants.rs index 242b10cf..cc3b094f 100644 --- a/src/common/concurrent/constants.rs +++ b/src/common/concurrent/constants.rs @@ -1,19 +1,23 @@ -pub(crate) const MAX_SYNC_REPEATS: usize = 4; -pub(crate) const PERIODICAL_SYNC_INITIAL_DELAY_MILLIS: u64 = 300; +pub(crate) const DEFAULT_MAX_LOG_SYNC_REPEATS: usize = 4; +pub(crate) const LOG_SYNC_INTERVAL_MILLIS: u64 = 300; pub(crate) const READ_LOG_FLUSH_POINT: usize = 64; -pub(crate) const READ_LOG_SIZE: usize = READ_LOG_FLUSH_POINT * (MAX_SYNC_REPEATS + 2); // 384 - pub(crate) const WRITE_LOG_FLUSH_POINT: usize = 64; -pub(crate) const WRITE_LOG_SIZE: usize = WRITE_LOG_FLUSH_POINT * (MAX_SYNC_REPEATS + 2); // 384 + +// 384 elements +pub(crate) const READ_LOG_CH_SIZE: usize = + READ_LOG_FLUSH_POINT * (DEFAULT_MAX_LOG_SYNC_REPEATS + 2); + +// 384 elements +pub(crate) const WRITE_LOG_CH_SIZE: usize = + WRITE_LOG_FLUSH_POINT * (DEFAULT_MAX_LOG_SYNC_REPEATS + 2); // TODO: Calculate the batch size based on the number of entries in the cache (or an // estimated number of entries to evict) -pub(crate) const EVICTION_BATCH_SIZE: usize = WRITE_LOG_SIZE; -pub(crate) const INVALIDATION_BATCH_SIZE: usize = WRITE_LOG_SIZE; +pub(crate) const DEFAULT_EVICTION_BATCH_SIZE: usize = WRITE_LOG_CH_SIZE; /// The default timeout duration for the `run_pending_tasks` method. -pub(crate) const DEFAULT_RUN_PENDING_TASKS_TIMEOUT_MILLIS: u64 = 100; +pub(crate) const DEFAULT_MAINTENANCE_TASK_TIMEOUT_MILLIS: u64 = 100; #[cfg(feature = "sync")] pub(crate) const WRITE_RETRY_INTERVAL_MICROS: u64 = 50; diff --git a/src/common/concurrent/housekeeper.rs b/src/common/concurrent/housekeeper.rs index 2ded85f7..706e34ad 100644 --- a/src/common/concurrent/housekeeper.rs +++ b/src/common/concurrent/housekeeper.rs @@ -1,13 +1,11 @@ -use super::constants::{ - DEFAULT_RUN_PENDING_TASKS_TIMEOUT_MILLIS, MAX_SYNC_REPEATS, - PERIODICAL_SYNC_INITIAL_DELAY_MILLIS, -}; +use super::constants::LOG_SYNC_INTERVAL_MILLIS; use super::{ atomic_time::AtomicInstant, constants::{READ_LOG_FLUSH_POINT, WRITE_LOG_FLUSH_POINT}, }; use crate::common::time::{CheckedTimeOps, Instant}; +use crate::common::HousekeeperConfig; use parking_lot::{Mutex, MutexGuard}; use std::{ @@ -17,7 +15,12 @@ use std::{ pub(crate) trait InnerSync { /// Runs the pending tasks. Returns `true` if there are more entries to evict. - fn run_pending_tasks(&self, max_sync_repeats: usize, timeout: Option) -> bool; + fn run_pending_tasks( + &self, + timeout: Option, + max_log_sync_repeats: usize, + eviction_batch_size: usize, + ) -> bool; fn now(&self) -> Instant; } @@ -25,11 +28,11 @@ pub(crate) trait InnerSync { pub(crate) struct Housekeeper { run_lock: Mutex<()>, run_after: AtomicInstant, - /// A flag to indicate if the last `run_pending_tasks` call left more entries to - /// evict. + /// A flag to indicate if the last call on `run_pending_tasks` method left some + /// entries to evict. /// /// Used only when the eviction listener closure is set for this cache instance - /// because, if not, `run_pending_tasks` will never leave more entries to evict. + /// because, if not, `run_pending_tasks` will never leave entries to evict. more_entries_to_evict: Option, /// The timeout duration for the `run_pending_tasks` method. This is a safe-guard /// to prevent cache read/write operations (that may call `run_pending_tasks` @@ -38,17 +41,21 @@ pub(crate) struct Housekeeper { /// /// Used only when the eviction listener closure is set for this cache instance. maintenance_task_timeout: Option, + /// The maximum repeat count for receiving operation logs from the read and write + /// log channels. Default: `MAX_LOG_SYNC_REPEATS`. + max_log_sync_repeats: usize, + /// The batch size of entries to be processed by each internal eviction method. + /// Default: `EVICTION_BATCH_SIZE`. + eviction_batch_size: usize, auto_run_enabled: AtomicBool, } impl Housekeeper { - pub(crate) fn new(is_eviction_listener_enabled: bool) -> Self { + pub(crate) fn new(is_eviction_listener_enabled: bool, config: HousekeeperConfig) -> Self { let (more_entries_to_evict, maintenance_task_timeout) = if is_eviction_listener_enabled { ( Some(AtomicBool::new(false)), - Some(Duration::from_millis( - DEFAULT_RUN_PENDING_TASKS_TIMEOUT_MILLIS, - )), + Some(config.maintenance_task_timeout), ) } else { (None, None) @@ -59,6 +66,8 @@ impl Housekeeper { run_after: AtomicInstant::new(Self::sync_after(Instant::now())), more_entries_to_evict, maintenance_task_timeout, + max_log_sync_repeats: config.max_log_sync_repeats, + eviction_batch_size: config.eviction_batch_size, auto_run_enabled: AtomicBool::new(true), } } @@ -109,12 +118,14 @@ impl Housekeeper { let now = cache.now(); self.run_after.set_instant(Self::sync_after(now)); let timeout = self.maintenance_task_timeout; - let more_to_evict = cache.run_pending_tasks(MAX_SYNC_REPEATS, timeout); + let repeats = self.max_log_sync_repeats; + let batch_size = self.eviction_batch_size; + let more_to_evict = cache.run_pending_tasks(timeout, repeats, batch_size); self.set_more_entries_to_evict(more_to_evict); } fn sync_after(now: Instant) -> Instant { - let dur = Duration::from_millis(PERIODICAL_SYNC_INITIAL_DELAY_MILLIS); + let dur = Duration::from_millis(LOG_SYNC_INTERVAL_MILLIS); let ts = now.checked_add(dur); // Assuming that `now` is current wall clock time, this should never fail at // least next millions of years. diff --git a/src/future/base_cache.rs b/src/future/base_cache.rs index 30b244bf..57e8d5d6 100644 --- a/src/future/base_cache.rs +++ b/src/future/base_cache.rs @@ -12,8 +12,7 @@ use crate::{ concurrent::{ atomic_time::AtomicInstant, constants::{ - EVICTION_BATCH_SIZE, INVALIDATION_BATCH_SIZE, READ_LOG_FLUSH_POINT, READ_LOG_SIZE, - WRITE_LOG_FLUSH_POINT, WRITE_LOG_SIZE, + READ_LOG_CH_SIZE, READ_LOG_FLUSH_POINT, WRITE_LOG_CH_SIZE, WRITE_LOG_FLUSH_POINT, }, deques::Deques, entry_info::EntryInfo, @@ -24,7 +23,7 @@ use crate::{ frequency_sketch::FrequencySketch, time::{CheckedTimeOps, Clock, Instant}, timer_wheel::{ReschedulingResult, TimerWheel}, - CacheRegion, + CacheRegion, HousekeeperConfig, }, future::CancelGuard, notification::{AsyncEvictionListener, RemovalCause}, @@ -171,12 +170,13 @@ where eviction_policy: EvictionPolicy, eviction_listener: Option>, expiration_policy: ExpirationPolicy, + housekeeper_config: HousekeeperConfig, invalidator_enabled: bool, ) -> Self { let (r_size, w_size) = if max_capacity == Some(0) { (0, 0) } else { - (READ_LOG_SIZE, WRITE_LOG_SIZE) + (READ_LOG_CH_SIZE, WRITE_LOG_CH_SIZE) }; let is_eviction_listener_enabled = eviction_listener.is_some(); @@ -204,7 +204,10 @@ where write_op_ch: w_snd, interrupted_op_ch_snd: i_snd, interrupted_op_ch_rcv: i_rcv, - housekeeper: Some(Arc::new(Housekeeper::new(is_eviction_listener_enabled))), + housekeeper: Some(Arc::new(Housekeeper::new( + is_eviction_listener_enabled, + housekeeper_config, + ))), } } @@ -1211,7 +1214,7 @@ where (1, 0) } else { let ic = initial_capacity - .map(|cap| cap + WRITE_LOG_SIZE) + .map(|cap| cap + WRITE_LOG_CH_SIZE) .unwrap_or_default(); (64, ic) }; @@ -1398,8 +1401,14 @@ where S: BuildHasher + Clone + Send + Sync + 'static, { /// Runs the pending tasks. Returns `true` if there are more entries to evict. - async fn run_pending_tasks(&self, max_repeats: usize, timeout: Option) -> bool { - self.do_run_pending_tasks(max_repeats, timeout).await + async fn run_pending_tasks( + &self, + timeout: Option, + max_log_sync_repeats: usize, + eviction_batch_size: usize, + ) -> bool { + self.do_run_pending_tasks(timeout, max_log_sync_repeats, eviction_batch_size) + .await } /// Notifies all the async tasks waiting in `BaseCache::schedule_write_op` method @@ -1420,7 +1429,12 @@ where S: BuildHasher + Clone + Send + Sync + 'static, { /// Runs the pending tasks. Returns `true` if there are more entries to evict. - async fn do_run_pending_tasks(&self, max_repeats: usize, timeout: Option) -> bool { + async fn do_run_pending_tasks( + &self, + timeout: Option, + max_log_sync_repeats: usize, + eviction_batch_size: usize, + ) -> bool { if self.max_capacity == Some(0) { return false; } @@ -1464,7 +1478,7 @@ where // method for the write op channel to have enough room, notify them. let listeners = self.write_op_ch_ready_event.total_listeners(); if listeners > 0 { - let n = listeners.min(WRITE_LOG_SIZE - self.write_op_ch.len()); + let n = listeners.min(WRITE_LOG_CH_SIZE - self.write_op_ch.len()); // Notify the `n` listeners. The `notify` method accepts 0, so no // need to check if `n` is greater than 0. self.write_op_ch_ready_event.notify(n); @@ -1492,7 +1506,7 @@ where self.evict_expired_entries_using_deqs( &mut deqs, &mut timer_wheel, - EVICTION_BATCH_SIZE, + eviction_batch_size, &mut eviction_state, ) .await; @@ -1506,7 +1520,7 @@ where invalidator, &mut deqs, &mut timer_wheel, - INVALIDATION_BATCH_SIZE, + eviction_batch_size, &mut eviction_state, ) .await; @@ -1519,7 +1533,7 @@ where self.evict_lru_entries( &mut deqs, &mut timer_wheel, - EVICTION_BATCH_SIZE, + eviction_batch_size, weights_to_evict, &mut eviction_state, ) @@ -1528,7 +1542,7 @@ where // Check whether to continue this loop or not. - let should_process_logs = calls <= max_repeats + let should_process_logs = calls <= max_log_sync_repeats && (self.read_op_ch.len() >= READ_LOG_FLUSH_POINT || self.write_op_ch.len() >= WRITE_LOG_FLUSH_POINT); @@ -2840,7 +2854,10 @@ fn is_expired_by_ttl( #[cfg(test)] mod tests { - use crate::policy::{EvictionPolicy, ExpirationPolicy}; + use crate::{ + common::HousekeeperConfig, + policy::{EvictionPolicy, ExpirationPolicy}, + }; use super::BaseCache; @@ -2862,6 +2879,7 @@ mod tests { EvictionPolicy::default(), None, ExpirationPolicy::default(), + HousekeeperConfig::default(), false, ); cache.inner.enable_frequency_sketch_for_testing().await; @@ -2949,19 +2967,19 @@ mod tests { ($cache:ident, $key:ident, $hash:ident, $mock:ident, $duration_secs:expr) => { // Increment the time. $mock.increment(Duration::from_millis($duration_secs * 1000 - 1)); - $cache.inner.do_run_pending_tasks(1, None).await; + $cache.inner.do_run_pending_tasks(None, 1, 10).await; assert!($cache.contains_key_with_hash(&$key, $hash)); assert_eq!($cache.entry_count(), 1); // Increment the time by 1ms (3). The entry should be expired. $mock.increment(Duration::from_millis(1)); - $cache.inner.do_run_pending_tasks(1, None).await; + $cache.inner.do_run_pending_tasks(None, 1, 10).await; assert!(!$cache.contains_key_with_hash(&$key, $hash)); // Increment the time again to ensure the entry has been evicted from the // cache. $mock.increment(Duration::from_secs(1)); - $cache.inner.do_run_pending_tasks(1, None).await; + $cache.inner.do_run_pending_tasks(None, 1, 10).await; assert_eq!($cache.entry_count(), 0); }; } @@ -3215,6 +3233,7 @@ mod tests { Some(Duration::from_secs(TTI)), expiry, ), + HousekeeperConfig::default(), false, ); cache.reconfigure_for_testing().await; @@ -3245,7 +3264,7 @@ mod tests { insert(&cache, key, hash, value).await; // Run a sync to register the entry to the internal data structures including // the timer wheel. - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; assert_eq!(cache.entry_count(), 1); assert_expiry!(cache, key, hash, mock, 1); @@ -3267,12 +3286,12 @@ mod tests { ExpiryExpectation::after_create(line!(), key, value, current_time(&cache), None); let inserted_at = current_time(&cache); insert(&cache, key, hash, value).await; - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(1)); - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; assert!(cache.contains_key_with_hash(&key, hash)); // Read the entry (2). @@ -3292,7 +3311,7 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; assert_expiry!(cache, key, hash, mock, 3); @@ -3314,12 +3333,12 @@ mod tests { ExpiryExpectation::after_create(line!(), key, value, current_time(&cache), None); let inserted_at = current_time(&cache); insert(&cache, key, hash, value).await; - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(1)); - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; assert!(cache.contains_key_with_hash(&key, hash)); // Read the entry (2). @@ -3339,11 +3358,11 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; // Increment the time. mock.increment(Duration::from_secs(2)); - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3358,7 +3377,7 @@ mod tests { Some(3), ); insert(&cache, key, hash, value).await; - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; assert_eq!(cache.entry_count(), 1); assert_expiry!(cache, key, hash, mock, 3); @@ -3381,12 +3400,12 @@ mod tests { ExpiryExpectation::after_create(line!(), key, value, current_time(&cache), None); let inserted_at = current_time(&cache); insert(&cache, key, hash, value).await; - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(1)); - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3407,11 +3426,11 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; // Increment the time. mock.increment(Duration::from_secs(2)); - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3426,7 +3445,7 @@ mod tests { None, ); insert(&cache, key, hash, value).await; - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; assert_eq!(cache.entry_count(), 1); assert_expiry!(cache, key, hash, mock, 7); @@ -3448,12 +3467,12 @@ mod tests { ExpiryExpectation::after_create(line!(), key, value, current_time(&cache), Some(8)); let inserted_at = current_time(&cache); insert(&cache, key, hash, value).await; - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(5)); - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3474,7 +3493,7 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; assert_expiry!(cache, key, hash, mock, 7); @@ -3496,12 +3515,12 @@ mod tests { ExpiryExpectation::after_create(line!(), key, value, current_time(&cache), Some(8)); let inserted_at = current_time(&cache); insert(&cache, key, hash, value).await; - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(5)); - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3522,11 +3541,11 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; // Increment the time. mock.increment(Duration::from_secs(6)); - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3547,7 +3566,7 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; assert_expiry!(cache, key, hash, mock, 5); @@ -3568,12 +3587,12 @@ mod tests { *expectation.lock().unwrap() = ExpiryExpectation::after_create(line!(), key, value, current_time(&cache), Some(9)); insert(&cache, key, hash, value).await; - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(6)); - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3589,12 +3608,12 @@ mod tests { ); let updated_at = current_time(&cache); insert(&cache, key, hash, value).await; - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(6)); - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3615,11 +3634,11 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; // Increment the time. mock.increment(Duration::from_secs(6)); - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3640,7 +3659,7 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.do_run_pending_tasks(1, None).await; + cache.inner.do_run_pending_tasks(None, 1, 10).await; assert_expiry!(cache, key, hash, mock, 4); } diff --git a/src/future/builder.rs b/src/future/builder.rs index 7c6943de..6a99c428 100644 --- a/src/future/builder.rs +++ b/src/future/builder.rs @@ -1,6 +1,6 @@ use super::{Cache, FutureExt}; use crate::{ - common::{builder_utils, concurrent::Weigher}, + common::{builder_utils, concurrent::Weigher, HousekeeperConfig}, notification::{AsyncEvictionListener, ListenerFuture, RemovalCause}, policy::{EvictionPolicy, ExpirationPolicy}, Expiry, @@ -63,6 +63,7 @@ pub struct CacheBuilder { eviction_policy: EvictionPolicy, eviction_listener: Option>, expiration_policy: ExpirationPolicy, + housekeeper_config: HousekeeperConfig, invalidator_enabled: bool, cache_type: PhantomData, } @@ -81,6 +82,7 @@ where eviction_policy: EvictionPolicy::default(), eviction_listener: None, expiration_policy: ExpirationPolicy::default(), + housekeeper_config: HousekeeperConfig::default(), invalidator_enabled: false, cache_type: PhantomData, } @@ -97,7 +99,7 @@ where pub fn new(max_capacity: u64) -> Self { Self { max_capacity: Some(max_capacity), - ..Default::default() + ..Self::default() } } @@ -121,6 +123,7 @@ where self.eviction_policy, self.eviction_listener, self.expiration_policy, + self.housekeeper_config, self.invalidator_enabled, ) } @@ -218,6 +221,7 @@ where self.eviction_policy, self.eviction_listener, self.expiration_policy, + self.housekeeper_config, self.invalidator_enabled, ) } @@ -394,6 +398,14 @@ impl CacheBuilder { builder } + #[cfg(test)] + pub(crate) fn housekeeper_config(self, conf: HousekeeperConfig) -> Self { + Self { + housekeeper_config: conf, + ..self + } + } + /// Enables support for [`Cache::invalidate_entries_if`][cache-invalidate-if] /// method. /// diff --git a/src/future/cache.rs b/src/future/cache.rs index 40112f88..812e26fb 100644 --- a/src/future/cache.rs +++ b/src/future/cache.rs @@ -5,7 +5,7 @@ use super::{ WriteOp, }; use crate::{ - common::concurrent::Weigher, + common::{concurrent::Weigher, HousekeeperConfig}, notification::AsyncEvictionListener, ops::compute::{self, CompResult}, policy::{EvictionPolicy, ExpirationPolicy}, @@ -789,6 +789,7 @@ where EvictionPolicy::default(), None, ExpirationPolicy::default(), + HousekeeperConfig::default(), false, ) } @@ -819,6 +820,7 @@ where eviction_policy: EvictionPolicy, eviction_listener: Option>, expiration_policy: ExpirationPolicy, + housekeeper_config: HousekeeperConfig, invalidator_enabled: bool, ) -> Self { Self { @@ -831,6 +833,7 @@ where eviction_policy, eviction_listener, expiration_policy, + housekeeper_config, invalidator_enabled, ), value_initializer: Arc::new(ValueInitializer::with_hasher(build_hasher)), @@ -2113,7 +2116,7 @@ fn never_ignore<'a, V>() -> Option<&'a mut fn(&V) -> bool> { mod tests { use super::Cache; use crate::{ - common::time::Clock, + common::{time::Clock, HousekeeperConfig}, future::FutureExt, notification::{ListenerFuture, RemovalCause}, ops::compute, @@ -2125,7 +2128,7 @@ mod tests { use std::{ convert::Infallible, sync::{ - atomic::{AtomicU32, Ordering}, + atomic::{AtomicU32, AtomicU8, Ordering}, Arc, }, time::{Duration, Instant as StdInstant}, @@ -5103,6 +5106,159 @@ mod tests { verify_notification_vec(&cache, actual, &expected).await; } + // When the eviction listener is not set, calling `run_pending_tasks` once should + // evict all entries that can be removed. + #[tokio::test] + async fn no_batch_size_limit_on_eviction() { + const MAX_CAPACITY: u64 = 20; + + const EVICTION_TIMEOUT: Duration = Duration::from_millis(30); + const MAX_LOG_SYNC_REPEATS: usize = 1; + const EVICTION_BATCH_SIZE: usize = 1; + + let hk_conf = HousekeeperConfig::new( + Some(EVICTION_TIMEOUT), + Some(MAX_LOG_SYNC_REPEATS), + Some(EVICTION_BATCH_SIZE), + ); + + // Create a cache with the LRU policy. + let mut cache = Cache::builder() + .max_capacity(MAX_CAPACITY) + .eviction_policy(EvictionPolicy::lru()) + .housekeeper_config(hk_conf) + .build(); + cache.reconfigure_for_testing().await; + + // Make the cache exterior immutable. + let cache = cache; + + // Fill the cache. + for i in 0..MAX_CAPACITY { + let v = format!("v{i}"); + cache.insert(i, v).await + } + // The max capacity should not change because we have not called + // `run_pending_tasks` yet. + assert_eq!(cache.entry_count(), 0); + + cache.run_pending_tasks().await; + assert_eq!(cache.entry_count(), MAX_CAPACITY); + + // Insert more items the cache. + for i in MAX_CAPACITY..(MAX_CAPACITY * 2) { + let v = format!("v{i}"); + cache.insert(i, v).await + } + // The max capacity should not change because we have not called + // `run_pending_tasks` yet. + assert_eq!(cache.entry_count(), MAX_CAPACITY); + // Both old and new keys should exist. + assert!(cache.contains_key(&0)); // old + assert!(cache.contains_key(&(MAX_CAPACITY - 1))); // old + assert!(cache.contains_key(&(MAX_CAPACITY * 2 - 1))); // new + + // Process the remaining write op logs (there should be MAX_CAPACITY logs), + // and evict the LRU entries. + cache.run_pending_tasks().await; + assert_eq!(cache.entry_count(), MAX_CAPACITY); + + // Now the old keys should be gone. + assert!(!cache.contains_key(&0)); + assert!(!cache.contains_key(&(MAX_CAPACITY - 1))); + // And the new keys should exist. + assert!(cache.contains_key(&(MAX_CAPACITY * 2 - 1))); + } + + #[tokio::test] + async fn slow_eviction_listener() { + const MAX_CAPACITY: u64 = 20; + + const EVICTION_TIMEOUT: Duration = Duration::from_millis(30); + const LISTENER_DELAY: Duration = Duration::from_millis(11); + const MAX_LOG_SYNC_REPEATS: usize = 1; + const EVICTION_BATCH_SIZE: usize = 1; + + let hk_conf = HousekeeperConfig::new( + Some(EVICTION_TIMEOUT), + Some(MAX_LOG_SYNC_REPEATS), + Some(EVICTION_BATCH_SIZE), + ); + + let (clock, mock) = Clock::mock(); + let listener_call_count = Arc::new(AtomicU8::new(0)); + let lcc = Arc::clone(&listener_call_count); + + // A slow eviction listener that spend `LISTENER_DELAY` to process a removal + // notification. + let listener = move |_k, _v, _cause| { + mock.increment(LISTENER_DELAY); + lcc.fetch_add(1, Ordering::AcqRel); + }; + + // Create a cache with the LRU policy. + let mut cache = Cache::builder() + .max_capacity(MAX_CAPACITY) + .eviction_policy(EvictionPolicy::lru()) + .eviction_listener(listener) + .housekeeper_config(hk_conf) + .build(); + cache.reconfigure_for_testing().await; + cache.set_expiration_clock(Some(clock)).await; + + // Make the cache exterior immutable. + let cache = cache; + + // Fill the cache. + for i in 0..MAX_CAPACITY { + let v = format!("v{i}"); + cache.insert(i, v).await + } + // The max capacity should not change because we have not called + // `run_pending_tasks` yet. + assert_eq!(cache.entry_count(), 0); + + cache.run_pending_tasks().await; + assert_eq!(listener_call_count.load(Ordering::Acquire), 0); + assert_eq!(cache.entry_count(), MAX_CAPACITY); + + // Insert more items the cache. + for i in MAX_CAPACITY..(MAX_CAPACITY * 2) { + let v = format!("v{i}"); + cache.insert(i, v).await + } + assert_eq!(cache.entry_count(), MAX_CAPACITY); + + cache.run_pending_tasks().await; + // Because of the slow listener, cache should get an over capacity. + let mut expected_call_count = 3; + assert_eq!( + listener_call_count.load(Ordering::Acquire) as u64, + expected_call_count + ); + assert_eq!(cache.entry_count(), MAX_CAPACITY * 2 - expected_call_count); + + loop { + cache.run_pending_tasks().await; + + expected_call_count += 3; + if expected_call_count > MAX_CAPACITY { + expected_call_count = MAX_CAPACITY; + } + + let actual_count = listener_call_count.load(Ordering::Acquire) as u64; + assert_eq!(actual_count, expected_call_count); + let expected_entry_count = MAX_CAPACITY * 2 - expected_call_count; + assert_eq!(cache.entry_count(), expected_entry_count); + + if expected_call_count >= MAX_CAPACITY { + break; + } + } + + assert_eq!(cache.entry_count(), MAX_CAPACITY); + } + // NOTE: To enable the panic logging, run the following command: // // RUST_LOG=moka=info cargo test --features 'future, logging' -- \ diff --git a/src/future/housekeeper.rs b/src/future/housekeeper.rs index 2fd81e44..d20cad31 100644 --- a/src/future/housekeeper.rs +++ b/src/future/housekeeper.rs @@ -1,12 +1,10 @@ use crate::common::{ concurrent::{ atomic_time::AtomicInstant, - constants::{ - DEFAULT_RUN_PENDING_TASKS_TIMEOUT_MILLIS, MAX_SYNC_REPEATS, - PERIODICAL_SYNC_INITIAL_DELAY_MILLIS, READ_LOG_FLUSH_POINT, WRITE_LOG_FLUSH_POINT, - }, + constants::{LOG_SYNC_INTERVAL_MILLIS, READ_LOG_FLUSH_POINT, WRITE_LOG_FLUSH_POINT}, }, time::{CheckedTimeOps, Instant}, + HousekeeperConfig, }; use std::{ @@ -27,7 +25,12 @@ use futures_util::future::{BoxFuture, Shared}; #[async_trait] pub(crate) trait InnerSync { /// Runs the pending tasks. Returns `true` if there are more entries to evict. - async fn run_pending_tasks(&self, max_sync_repeats: usize, timeout: Option) -> bool; + async fn run_pending_tasks( + &self, + timeout: Option, + max_log_sync_repeats: usize, + eviction_batch_size: usize, + ) -> bool; /// Notifies all the async tasks waiting in `BaseCache::schedule_write_op` method /// for the write op channel to have enough room. @@ -40,11 +43,11 @@ pub(crate) struct Housekeeper { /// A shared `Future` of the maintenance task that is currently being resolved. current_task: Mutex>>>, run_after: AtomicInstant, - /// A flag to indicate if the last `run_pending_tasks` call left more entries to - /// evict. + /// A flag to indicate if the last call on `run_pending_tasks` method left some + /// entries to evict. /// /// Used only when the eviction listener closure is set for this cache instance - /// because, if not, `run_pending_tasks` will never leave more entries to evict. + /// because, if not, `run_pending_tasks` will never leave entries to evict. more_entries_to_evict: Option, /// The timeout duration for the `run_pending_tasks` method. This is a safe-guard /// to prevent cache read/write operations (that may call `run_pending_tasks` @@ -53,6 +56,12 @@ pub(crate) struct Housekeeper { /// /// Used only when the eviction listener closure is set for this cache instance. maintenance_task_timeout: Option, + /// The maximum repeat count for receiving operation logs from the read and write + /// log channels. Default: `MAX_LOG_SYNC_REPEATS`. + max_log_sync_repeats: usize, + /// The batch size of entries to be processed by each internal eviction method. + /// Default: `EVICTION_BATCH_SIZE`. + eviction_batch_size: usize, auto_run_enabled: AtomicBool, #[cfg(test)] pub(crate) start_count: AtomicUsize, @@ -61,13 +70,11 @@ pub(crate) struct Housekeeper { } impl Housekeeper { - pub(crate) fn new(is_eviction_listener_enabled: bool) -> Self { + pub(crate) fn new(is_eviction_listener_enabled: bool, config: HousekeeperConfig) -> Self { let (more_entries_to_evict, maintenance_task_timeout) = if is_eviction_listener_enabled { ( Some(AtomicBool::new(false)), - Some(Duration::from_millis( - DEFAULT_RUN_PENDING_TASKS_TIMEOUT_MILLIS, - )), + Some(config.maintenance_task_timeout), ) } else { (None, None) @@ -78,6 +85,8 @@ impl Housekeeper { run_after: AtomicInstant::new(Self::sync_after(Instant::now())), more_entries_to_evict, maintenance_task_timeout, + max_log_sync_repeats: config.max_log_sync_repeats, + eviction_batch_size: config.eviction_batch_size, auto_run_enabled: AtomicBool::new(true), #[cfg(test)] start_count: Default::default(), @@ -168,8 +177,10 @@ impl Housekeeper { more_to_evict = task.clone().await; } else { let timeout = self.maintenance_task_timeout; + let repeats = self.max_log_sync_repeats; + let batch_size = self.eviction_batch_size; // Create a new maintenance task and await it. - let task = async move { cache.run_pending_tasks(MAX_SYNC_REPEATS, timeout).await } + let task = async move { cache.run_pending_tasks(timeout, repeats, batch_size).await } .boxed() .shared(); *current_task = Some(task.clone()); @@ -191,7 +202,7 @@ impl Housekeeper { } fn sync_after(now: Instant) -> Instant { - let dur = Duration::from_millis(PERIODICAL_SYNC_INITIAL_DELAY_MILLIS); + let dur = Duration::from_millis(LOG_SYNC_INTERVAL_MILLIS); let ts = now.checked_add(dur); // Assuming that `now` is current wall clock time, this should never fail at // least next millions of years. diff --git a/src/sync/builder.rs b/src/sync/builder.rs index cd26dd23..4fec063e 100644 --- a/src/sync/builder.rs +++ b/src/sync/builder.rs @@ -1,6 +1,6 @@ use super::{Cache, SegmentedCache}; use crate::{ - common::{builder_utils, concurrent::Weigher}, + common::{builder_utils, concurrent::Weigher, HousekeeperConfig}, notification::{EvictionListener, RemovalCause}, policy::{EvictionPolicy, ExpirationPolicy}, Expiry, @@ -56,6 +56,7 @@ pub struct CacheBuilder { eviction_policy: EvictionPolicy, eviction_listener: Option>, expiration_policy: ExpirationPolicy, + housekeeper_config: HousekeeperConfig, invalidator_enabled: bool, cache_type: PhantomData, } @@ -75,6 +76,7 @@ where eviction_listener: None, eviction_policy: EvictionPolicy::default(), expiration_policy: ExpirationPolicy::default(), + housekeeper_config: HousekeeperConfig::default(), invalidator_enabled: false, cache_type: PhantomData, } @@ -115,6 +117,7 @@ where eviction_policy: self.eviction_policy, eviction_listener: self.eviction_listener, expiration_policy: self.expiration_policy, + housekeeper_config: self.housekeeper_config, invalidator_enabled: self.invalidator_enabled, cache_type: PhantomData, } @@ -143,6 +146,7 @@ where self.eviction_policy, self.eviction_listener, self.expiration_policy, + self.housekeeper_config, self.invalidator_enabled, ) } @@ -230,6 +234,7 @@ where self.eviction_policy, self.eviction_listener, self.expiration_policy, + self.housekeeper_config, self.invalidator_enabled, ) } @@ -264,6 +269,7 @@ where self.eviction_policy, self.eviction_listener, self.expiration_policy, + self.housekeeper_config, self.invalidator_enabled, ) } @@ -353,6 +359,7 @@ where self.eviction_policy, self.eviction_listener, self.expiration_policy, + self.housekeeper_config, self.invalidator_enabled, ) } @@ -476,6 +483,14 @@ impl CacheBuilder { builder } + #[cfg(test)] + pub(crate) fn housekeeper_config(self, conf: HousekeeperConfig) -> Self { + Self { + housekeeper_config: conf, + ..self + } + } + /// Enables support for [`Cache::invalidate_entries_if`][cache-invalidate-if] /// method. /// diff --git a/src/sync/cache.rs b/src/sync/cache.rs index af3547f1..bd231251 100644 --- a/src/sync/cache.rs +++ b/src/sync/cache.rs @@ -8,6 +8,7 @@ use crate::{ constants::WRITE_RETRY_INTERVAL_MICROS, housekeeper::InnerSync, Weigher, WriteOp, }, time::Instant, + HousekeeperConfig, }, notification::EvictionListener, ops::compute::{self, CompResult}, @@ -709,6 +710,7 @@ where EvictionPolicy::default(), None, ExpirationPolicy::default(), + HousekeeperConfig::default(), false, ) } @@ -739,6 +741,7 @@ where eviction_policy: EvictionPolicy, eviction_listener: Option>, expiration_policy: ExpirationPolicy, + housekeeper_config: HousekeeperConfig, invalidator_enabled: bool, ) -> Self { Self { @@ -751,6 +754,7 @@ where eviction_policy, eviction_listener, expiration_policy, + housekeeper_config, invalidator_enabled, ), value_initializer: Arc::new(ValueInitializer::with_hasher(build_hasher)), @@ -1906,7 +1910,7 @@ where mod tests { use super::Cache; use crate::{ - common::time::Clock, + common::{time::Clock, HousekeeperConfig}, notification::RemovalCause, policy::{test_utils::ExpiryCallCounters, EvictionPolicy}, Expiry, @@ -1915,7 +1919,10 @@ mod tests { use parking_lot::Mutex; use std::{ convert::Infallible, - sync::Arc, + sync::{ + atomic::{AtomicU8, Ordering}, + Arc, + }, time::{Duration, Instant as StdInstant}, }; @@ -4789,6 +4796,159 @@ mod tests { assert!(cache.key_locks_map_is_empty()); } + // When the eviction listener is not set, calling `run_pending_tasks` once should + // evict all entries that can be removed. + #[test] + fn no_batch_size_limit_on_eviction() { + const MAX_CAPACITY: u64 = 20; + + const EVICTION_TIMEOUT: Duration = Duration::from_millis(30); + const MAX_LOG_SYNC_REPEATS: usize = 1; + const EVICTION_BATCH_SIZE: usize = 1; + + let hk_conf = HousekeeperConfig::new( + Some(EVICTION_TIMEOUT), + Some(MAX_LOG_SYNC_REPEATS), + Some(EVICTION_BATCH_SIZE), + ); + + // Create a cache with the LRU policy. + let mut cache = Cache::builder() + .max_capacity(MAX_CAPACITY) + .eviction_policy(EvictionPolicy::lru()) + .housekeeper_config(hk_conf) + .build(); + cache.reconfigure_for_testing(); + + // Make the cache exterior immutable. + let cache = cache; + + // Fill the cache. + for i in 0..MAX_CAPACITY { + let v = format!("v{i}"); + cache.insert(i, v) + } + // The max capacity should not change because we have not called + // `run_pending_tasks` yet. + assert_eq!(cache.entry_count(), 0); + + cache.run_pending_tasks(); + assert_eq!(cache.entry_count(), MAX_CAPACITY); + + // Insert more items the cache. + for i in MAX_CAPACITY..(MAX_CAPACITY * 2) { + let v = format!("v{i}"); + cache.insert(i, v) + } + // The max capacity should not change because we have not called + // `run_pending_tasks` yet. + assert_eq!(cache.entry_count(), MAX_CAPACITY); + // Both old and new keys should exist. + assert!(cache.contains_key(&0)); // old + assert!(cache.contains_key(&(MAX_CAPACITY - 1))); // old + assert!(cache.contains_key(&(MAX_CAPACITY * 2 - 1))); // new + + // Process the remaining write op logs (there should be MAX_CAPACITY logs), + // and evict the LRU entries. + cache.run_pending_tasks(); + assert_eq!(cache.entry_count(), MAX_CAPACITY); + + // Now the old keys should be gone. + assert!(!cache.contains_key(&0)); + assert!(!cache.contains_key(&(MAX_CAPACITY - 1))); + // And the new keys should exist. + assert!(cache.contains_key(&(MAX_CAPACITY * 2 - 1))); + } + + #[test] + fn slow_eviction_listener() { + const MAX_CAPACITY: u64 = 20; + + const EVICTION_TIMEOUT: Duration = Duration::from_millis(30); + const LISTENER_DELAY: Duration = Duration::from_millis(11); + const MAX_LOG_SYNC_REPEATS: usize = 1; + const EVICTION_BATCH_SIZE: usize = 1; + + let hk_conf = HousekeeperConfig::new( + Some(EVICTION_TIMEOUT), + Some(MAX_LOG_SYNC_REPEATS), + Some(EVICTION_BATCH_SIZE), + ); + + let (clock, mock) = Clock::mock(); + let listener_call_count = Arc::new(AtomicU8::new(0)); + let lcc = Arc::clone(&listener_call_count); + + // A slow eviction listener that spend `LISTENER_DELAY` to process a removal + // notification. + let listener = move |_k, _v, _cause| { + mock.increment(LISTENER_DELAY); + lcc.fetch_add(1, Ordering::AcqRel); + }; + + // Create a cache with the LRU policy. + let mut cache = Cache::builder() + .max_capacity(MAX_CAPACITY) + .eviction_policy(EvictionPolicy::lru()) + .eviction_listener(listener) + .housekeeper_config(hk_conf) + .build(); + cache.reconfigure_for_testing(); + cache.set_expiration_clock(Some(clock)); + + // Make the cache exterior immutable. + let cache = cache; + + // Fill the cache. + for i in 0..MAX_CAPACITY { + let v = format!("v{i}"); + cache.insert(i, v) + } + // The max capacity should not change because we have not called + // `run_pending_tasks` yet. + assert_eq!(cache.entry_count(), 0); + + cache.run_pending_tasks(); + assert_eq!(listener_call_count.load(Ordering::Acquire), 0); + assert_eq!(cache.entry_count(), MAX_CAPACITY); + + // Insert more items the cache. + for i in MAX_CAPACITY..(MAX_CAPACITY * 2) { + let v = format!("v{i}"); + cache.insert(i, v); + } + assert_eq!(cache.entry_count(), MAX_CAPACITY); + + cache.run_pending_tasks(); + // Because of the slow listener, cache should get an over capacity. + let mut expected_call_count = 3; + assert_eq!( + listener_call_count.load(Ordering::Acquire) as u64, + expected_call_count + ); + assert_eq!(cache.entry_count(), MAX_CAPACITY * 2 - expected_call_count); + + loop { + cache.run_pending_tasks(); + + expected_call_count += 3; + if expected_call_count > MAX_CAPACITY { + expected_call_count = MAX_CAPACITY; + } + + let actual_count = listener_call_count.load(Ordering::Acquire) as u64; + assert_eq!(actual_count, expected_call_count); + let expected_entry_count = MAX_CAPACITY * 2 - expected_call_count; + assert_eq!(cache.entry_count(), expected_entry_count); + + if expected_call_count >= MAX_CAPACITY { + break; + } + } + + assert_eq!(cache.entry_count(), MAX_CAPACITY); + } + // NOTE: To enable the panic logging, run the following command: // // RUST_LOG=moka=info cargo test --features 'logging' -- \ diff --git a/src/sync/segment.rs b/src/sync/segment.rs index c22acd9a..f3bd626b 100644 --- a/src/sync/segment.rs +++ b/src/sync/segment.rs @@ -1,6 +1,7 @@ use super::{cache::Cache, CacheBuilder, OwnedKeyEntrySelector, RefKeyEntrySelector}; +use crate::common::concurrent::Weigher; use crate::{ - common::concurrent::Weigher, + common::HousekeeperConfig, notification::EvictionListener, policy::{EvictionPolicy, ExpirationPolicy}, sync_base::iter::{Iter, ScanningGet}, @@ -105,6 +106,7 @@ where EvictionPolicy::default(), None, ExpirationPolicy::default(), + HousekeeperConfig::default(), false, ) } @@ -211,6 +213,7 @@ where eviction_policy: EvictionPolicy, eviction_listener: Option>, expiration_policy: ExpirationPolicy, + housekeeper_config: HousekeeperConfig, invalidator_enabled: bool, ) -> Self { Self { @@ -224,6 +227,7 @@ where eviction_policy, eviction_listener, expiration_policy, + housekeeper_config, invalidator_enabled, )), } @@ -735,6 +739,7 @@ where eviction_policy: EvictionPolicy, eviction_listener: Option>, expiration_policy: ExpirationPolicy, + housekeeper_config: HousekeeperConfig, invalidator_enabled: bool, ) -> Self { assert!(num_segments > 0); @@ -758,6 +763,7 @@ where eviction_policy.clone(), eviction_listener.clone(), expiration_policy.clone(), + housekeeper_config.clone(), invalidator_enabled, ) }) diff --git a/src/sync_base/base_cache.rs b/src/sync_base/base_cache.rs index 61d557cc..fe6cc9ab 100644 --- a/src/sync_base/base_cache.rs +++ b/src/sync_base/base_cache.rs @@ -11,8 +11,7 @@ use crate::{ concurrent::{ atomic_time::AtomicInstant, constants::{ - EVICTION_BATCH_SIZE, INVALIDATION_BATCH_SIZE, READ_LOG_FLUSH_POINT, READ_LOG_SIZE, - WRITE_LOG_FLUSH_POINT, WRITE_LOG_SIZE, + READ_LOG_CH_SIZE, READ_LOG_FLUSH_POINT, WRITE_LOG_CH_SIZE, WRITE_LOG_FLUSH_POINT, }, deques::Deques, entry_info::EntryInfo, @@ -24,7 +23,7 @@ use crate::{ frequency_sketch::FrequencySketch, time::{CheckedTimeOps, Clock, Instant}, timer_wheel::{ReschedulingResult, TimerWheel}, - CacheRegion, + CacheRegion, HousekeeperConfig, }, notification::{notifier::RemovalNotifier, EvictionListener, RemovalCause}, policy::{EvictionPolicy, EvictionPolicyConfig, ExpirationPolicy}, @@ -146,12 +145,13 @@ where eviction_policy: EvictionPolicy, eviction_listener: Option>, expiration_policy: ExpirationPolicy, + housekeeper_config: HousekeeperConfig, invalidator_enabled: bool, ) -> Self { let (r_size, w_size) = if max_capacity == Some(0) { (0, 0) } else { - (READ_LOG_SIZE, WRITE_LOG_SIZE) + (READ_LOG_CH_SIZE, WRITE_LOG_CH_SIZE) }; let is_eviction_listener_enabled = eviction_listener.is_some(); @@ -176,7 +176,10 @@ where inner, read_op_ch: r_snd, write_op_ch: w_snd, - housekeeper: Some(Arc::new(Housekeeper::new(is_eviction_listener_enabled))), + housekeeper: Some(Arc::new(Housekeeper::new( + is_eviction_listener_enabled, + housekeeper_config, + ))), } } @@ -1058,7 +1061,7 @@ where (1, 0) } else { let ic = initial_capacity - .map(|cap| cap + WRITE_LOG_SIZE) + .map(|cap| cap + WRITE_LOG_CH_SIZE) .unwrap_or_default(); (64, ic) }; @@ -1236,8 +1239,13 @@ where V: Clone + Send + Sync + 'static, S: BuildHasher + Clone + Send + Sync + 'static, { - fn run_pending_tasks(&self, max_repeats: usize, timeout: Option) -> bool { - self.do_run_pending_tasks(max_repeats, timeout) + fn run_pending_tasks( + &self, + timeout: Option, + max_log_sync_repeats: usize, + eviction_batch_size: usize, + ) -> bool { + self.do_run_pending_tasks(timeout, max_log_sync_repeats, eviction_batch_size) } fn now(&self) -> Instant { @@ -1251,7 +1259,12 @@ where V: Clone + Send + Sync + 'static, S: BuildHasher + Clone + Send + Sync + 'static, { - fn do_run_pending_tasks(&self, max_repeats: usize, timeout: Option) -> bool { + fn do_run_pending_tasks( + &self, + timeout: Option, + max_log_sync_repeats: usize, + eviction_batch_size: usize, + ) -> bool { if self.max_capacity == Some(0) { return false; } @@ -1311,7 +1324,7 @@ where self.evict_expired_entries_using_deqs( &mut deqs, &mut timer_wheel, - EVICTION_BATCH_SIZE, + eviction_batch_size, &mut eviction_state, ); } @@ -1324,7 +1337,7 @@ where invalidator, &mut deqs, &mut timer_wheel, - INVALIDATION_BATCH_SIZE, + eviction_batch_size, &mut eviction_state, ); } @@ -1336,7 +1349,7 @@ where self.evict_lru_entries( &mut deqs, &mut timer_wheel, - EVICTION_BATCH_SIZE, + eviction_batch_size, weights_to_evict, &mut eviction_state, ); @@ -1344,7 +1357,7 @@ where // Check whether to continue this loop or not. - let should_process_logs = calls <= max_repeats + let should_process_logs = calls <= max_log_sync_repeats && (self.read_op_ch.len() >= READ_LOG_FLUSH_POINT || self.write_op_ch.len() >= WRITE_LOG_FLUSH_POINT); @@ -2565,7 +2578,10 @@ fn is_expired_by_ttl( #[cfg(test)] mod tests { - use crate::policy::{EvictionPolicy, ExpirationPolicy}; + use crate::{ + common::HousekeeperConfig, + policy::{EvictionPolicy, ExpirationPolicy}, + }; use super::BaseCache; @@ -2587,6 +2603,7 @@ mod tests { EvictionPolicy::default(), None, ExpirationPolicy::default(), + HousekeeperConfig::default(), false, ); cache.inner.enable_frequency_sketch_for_testing(); @@ -2671,19 +2688,19 @@ mod tests { ($cache:ident, $key:ident, $hash:ident, $mock:ident, $duration_secs:expr) => { // Increment the time. $mock.increment(Duration::from_millis($duration_secs * 1000 - 1)); - $cache.inner.run_pending_tasks(1, None); + $cache.inner.run_pending_tasks(None, 1, 10); assert!($cache.contains_key_with_hash(&$key, $hash)); assert_eq!($cache.entry_count(), 1); // Increment the time by 1ms (3). The entry should be expired. $mock.increment(Duration::from_millis(1)); - $cache.inner.run_pending_tasks(1, None); + $cache.inner.run_pending_tasks(None, 1, 10); assert!(!$cache.contains_key_with_hash(&$key, $hash)); // Increment the time again to ensure the entry has been evicted from the // cache. $mock.increment(Duration::from_secs(1)); - $cache.inner.run_pending_tasks(1, None); + $cache.inner.run_pending_tasks(None, 1, 10); assert_eq!($cache.entry_count(), 0); }; } @@ -2937,6 +2954,7 @@ mod tests { Some(Duration::from_secs(TTI)), expiry, ), + HousekeeperConfig::default(), false, ); cache.reconfigure_for_testing(); @@ -2967,7 +2985,7 @@ mod tests { insert(&cache, key, hash, value); // Run a sync to register the entry to the internal data structures including // the timer wheel. - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); assert_eq!(cache.entry_count(), 1); assert_expiry!(cache, key, hash, mock, 1); @@ -2989,12 +3007,12 @@ mod tests { ExpiryExpectation::after_create(line!(), key, value, current_time(&cache), None); let inserted_at = current_time(&cache); insert(&cache, key, hash, value); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(1)); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); assert!(cache.contains_key_with_hash(&key, hash)); // Read the entry (2). @@ -3013,7 +3031,7 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); assert_expiry!(cache, key, hash, mock, 3); @@ -3035,12 +3053,12 @@ mod tests { ExpiryExpectation::after_create(line!(), key, value, current_time(&cache), None); let inserted_at = current_time(&cache); insert(&cache, key, hash, value); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(1)); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); assert!(cache.contains_key_with_hash(&key, hash)); // Read the entry (2). @@ -3059,11 +3077,11 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); // Increment the time. mock.increment(Duration::from_secs(2)); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3078,7 +3096,7 @@ mod tests { Some(3), ); insert(&cache, key, hash, value); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); assert_eq!(cache.entry_count(), 1); assert_expiry!(cache, key, hash, mock, 3); @@ -3101,12 +3119,12 @@ mod tests { ExpiryExpectation::after_create(line!(), key, value, current_time(&cache), None); let inserted_at = current_time(&cache); insert(&cache, key, hash, value); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(1)); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3126,11 +3144,11 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); // Increment the time. mock.increment(Duration::from_secs(2)); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3145,7 +3163,7 @@ mod tests { None, ); insert(&cache, key, hash, value); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); assert_eq!(cache.entry_count(), 1); assert_expiry!(cache, key, hash, mock, 7); @@ -3167,12 +3185,12 @@ mod tests { ExpiryExpectation::after_create(line!(), key, value, current_time(&cache), Some(8)); let inserted_at = current_time(&cache); insert(&cache, key, hash, value); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(5)); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3192,7 +3210,7 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); assert_expiry!(cache, key, hash, mock, 7); @@ -3214,12 +3232,12 @@ mod tests { ExpiryExpectation::after_create(line!(), key, value, current_time(&cache), Some(8)); let inserted_at = current_time(&cache); insert(&cache, key, hash, value); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(5)); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3239,11 +3257,11 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); // Increment the time. mock.increment(Duration::from_secs(6)); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3263,7 +3281,7 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); assert_expiry!(cache, key, hash, mock, 5); @@ -3284,12 +3302,12 @@ mod tests { *expectation.lock().unwrap() = ExpiryExpectation::after_create(line!(), key, value, current_time(&cache), Some(9)); insert(&cache, key, hash, value); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(6)); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3305,12 +3323,12 @@ mod tests { ); let updated_at = current_time(&cache); insert(&cache, key, hash, value); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); assert_eq!(cache.entry_count(), 1); // Increment the time. mock.increment(Duration::from_secs(6)); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3330,11 +3348,11 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); // Increment the time. mock.increment(Duration::from_secs(6)); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); assert!(cache.contains_key_with_hash(&key, hash)); assert_eq!(cache.entry_count(), 1); @@ -3354,7 +3372,7 @@ mod tests { .map(Entry::into_value), Some(value) ); - cache.inner.run_pending_tasks(1, None); + cache.inner.run_pending_tasks(None, 1, 10); assert_expiry!(cache, key, hash, mock, 4); } From b9923f8aeb0f342ff7bed7b1c271701d6fbed6f6 Mon Sep 17 00:00:00 2001 From: Tatsuya Kawano Date: Sun, 14 Apr 2024 19:23:20 +0800 Subject: [PATCH 4/9] Bump the version to `v0.12.7` --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 15b67e1d..1b1770e2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "moka" -version = "0.12.6" +version = "0.12.7" edition = "2021" # Rust 1.65 was released on Nov 3, 2022. rust-version = "1.65" From b59b93892a21d6bf047a222a51b3211263dcf2b6 Mon Sep 17 00:00:00 2001 From: Tatsuya Kawano Date: Sun, 14 Apr 2024 19:23:27 +0800 Subject: [PATCH 5/9] Update the change log --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index df7f951d..6643892e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,13 @@ # Moka Cache — Change Log +## Version 0.12.7 + +### Changed + +- Ensure a single call to `run_pending_tasks` to evict as many entries as possible + from the cache ([#417](gh-pull-0417). + + ## Version 0.12.6 ### Fixed From 91c8497d0d92c8dec723b799eaec7f3db52f9829 Mon Sep 17 00:00:00 2001 From: Tatsuya Kawano Date: Sun, 14 Apr 2024 22:00:23 +0800 Subject: [PATCH 6/9] Update the change log --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6643892e..2890662e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ ### Changed - Ensure a single call to `run_pending_tasks` to evict as many entries as possible - from the cache ([#417](gh-pull-0417). + from the cache ([#417][gh-pull-0417]). ## Version 0.12.6 @@ -880,6 +880,7 @@ The minimum supported Rust version (MSRV) is now 1.51.0 (Mar 25, 2021). [gh-issue-0034]: https://github.com/moka-rs/moka/issues/34/ [gh-issue-0031]: https://github.com/moka-rs/moka/issues/31/ +[gh-pull-0417]: https://github.com/moka-rs/moka/pull/417/ [gh-pull-0390]: https://github.com/moka-rs/moka/pull/390/ [gh-pull-0384]: https://github.com/moka-rs/moka/pull/384/ [gh-pull-0382]: https://github.com/moka-rs/moka/pull/382/ From c6a4d4b5c226518687b4245481046691c4e74894 Mon Sep 17 00:00:00 2001 From: Tatsuya Kawano Date: Mon, 15 Apr 2024 22:49:36 +0800 Subject: [PATCH 7/9] Make a single call to `run_pending_tasks` to evict as many entries as possible from the cache Ensure the loop in the `do_run_pending_tasks` method is eventually stopped. --- src/common.rs | 13 ++- src/common/concurrent/constants.rs | 2 +- src/common/concurrent/housekeeper.rs | 11 +- src/future/base_cache.rs | 158 +++++++++++++++++---------- src/future/cache.rs | 8 +- src/future/housekeeper.rs | 17 +-- src/sync/cache.rs | 8 +- src/sync_base/base_cache.rs | 102 ++++++++++++----- 8 files changed, 208 insertions(+), 111 deletions(-) diff --git a/src/common.rs b/src/common.rs index 9748e86f..8657693c 100644 --- a/src/common.rs +++ b/src/common.rs @@ -76,10 +76,10 @@ pub(crate) struct HousekeeperConfig { pub(crate) maintenance_task_timeout: Duration, /// The maximum repeat count for receiving operation logs from the read and write /// log channels. Default: `MAX_LOG_SYNC_REPEATS`. - pub(crate) max_log_sync_repeats: usize, + pub(crate) max_log_sync_repeats: u32, /// The batch size of entries to be processed by each internal eviction method. /// Default: `EVICTION_BATCH_SIZE`. - pub(crate) eviction_batch_size: usize, + pub(crate) eviction_batch_size: u32, } impl Default for HousekeeperConfig { @@ -88,7 +88,7 @@ impl Default for HousekeeperConfig { maintenance_task_timeout: Duration::from_millis( DEFAULT_MAINTENANCE_TASK_TIMEOUT_MILLIS, ), - max_log_sync_repeats: DEFAULT_MAX_LOG_SYNC_REPEATS, + max_log_sync_repeats: DEFAULT_MAX_LOG_SYNC_REPEATS as u32, eviction_batch_size: DEFAULT_EVICTION_BATCH_SIZE, } } @@ -98,14 +98,15 @@ impl HousekeeperConfig { #[cfg(test)] pub(crate) fn new( maintenance_task_timeout: Option, - max_log_sync_repeats: Option, - eviction_batch_size: Option, + max_log_sync_repeats: Option, + eviction_batch_size: Option, ) -> Self { Self { maintenance_task_timeout: maintenance_task_timeout.unwrap_or(Duration::from_millis( DEFAULT_MAINTENANCE_TASK_TIMEOUT_MILLIS, )), - max_log_sync_repeats: max_log_sync_repeats.unwrap_or(DEFAULT_MAX_LOG_SYNC_REPEATS), + max_log_sync_repeats: max_log_sync_repeats + .unwrap_or(DEFAULT_MAX_LOG_SYNC_REPEATS as u32), eviction_batch_size: eviction_batch_size.unwrap_or(DEFAULT_EVICTION_BATCH_SIZE), } } diff --git a/src/common/concurrent/constants.rs b/src/common/concurrent/constants.rs index cc3b094f..629c8d6d 100644 --- a/src/common/concurrent/constants.rs +++ b/src/common/concurrent/constants.rs @@ -14,7 +14,7 @@ pub(crate) const WRITE_LOG_CH_SIZE: usize = // TODO: Calculate the batch size based on the number of entries in the cache (or an // estimated number of entries to evict) -pub(crate) const DEFAULT_EVICTION_BATCH_SIZE: usize = WRITE_LOG_CH_SIZE; +pub(crate) const DEFAULT_EVICTION_BATCH_SIZE: u32 = WRITE_LOG_CH_SIZE as u32; /// The default timeout duration for the `run_pending_tasks` method. pub(crate) const DEFAULT_MAINTENANCE_TASK_TIMEOUT_MILLIS: u64 = 100; diff --git a/src/common/concurrent/housekeeper.rs b/src/common/concurrent/housekeeper.rs index 706e34ad..11df87f4 100644 --- a/src/common/concurrent/housekeeper.rs +++ b/src/common/concurrent/housekeeper.rs @@ -14,12 +14,13 @@ use std::{ }; pub(crate) trait InnerSync { - /// Runs the pending tasks. Returns `true` if there are more entries to evict. + /// Runs the pending tasks. Returns `true` if there are more entries to evict in + /// next run. fn run_pending_tasks( &self, timeout: Option, - max_log_sync_repeats: usize, - eviction_batch_size: usize, + max_log_sync_repeats: u32, + eviction_batch_size: u32, ) -> bool; fn now(&self) -> Instant; @@ -43,10 +44,10 @@ pub(crate) struct Housekeeper { maintenance_task_timeout: Option, /// The maximum repeat count for receiving operation logs from the read and write /// log channels. Default: `MAX_LOG_SYNC_REPEATS`. - max_log_sync_repeats: usize, + max_log_sync_repeats: u32, /// The batch size of entries to be processed by each internal eviction method. /// Default: `EVICTION_BATCH_SIZE`. - eviction_batch_size: usize, + eviction_batch_size: u32, auto_run_enabled: AtomicBool, } diff --git a/src/future/base_cache.rs b/src/future/base_cache.rs index 57e8d5d6..4881cd2b 100644 --- a/src/future/base_cache.rs +++ b/src/future/base_cache.rs @@ -384,7 +384,7 @@ where #[inline] pub(crate) async fn apply_reads_writes_if_needed( - inner: Arc, + inner: &Arc, ch: &Sender>, now: Instant, housekeeper: Option<&HouseKeeperArc>, @@ -466,8 +466,7 @@ where op: ReadOp, now: Instant, ) -> Result<(), TrySendError>> { - self.apply_reads_if_needed(Arc::clone(&self.inner), now) - .await; + self.apply_reads_if_needed(&self.inner, now).await; let ch = &self.read_op_ch; match ch.try_send(op) { // Discard the ReadOp when the channel is full. @@ -644,13 +643,7 @@ where let mut spin_loop_attempts = 0u8; loop { // Run the `Inner::do_run_pending_tasks` method if needed. - BaseCache::::apply_reads_writes_if_needed( - Arc::clone(inner), - ch, - ts, - housekeeper, - ) - .await; + BaseCache::::apply_reads_writes_if_needed(inner, ch, ts, housekeeper).await; // Try to send our op to the write op channel. match ch.try_send(op) { @@ -736,7 +729,7 @@ where } #[inline] - async fn apply_reads_if_needed(&self, inner: Arc>, now: Instant) { + async fn apply_reads_if_needed(&self, inner: &Arc>, now: Instant) { let len = self.read_op_ch.len(); if let Some(hk) = &self.housekeeper { @@ -926,6 +919,7 @@ impl<'a, K, V> EvictionState<'a, K, V> { struct EvictionCounters { entry_count: u64, weighted_size: u64, + eviction_count: u64, } impl EvictionCounters { @@ -934,6 +928,7 @@ impl EvictionCounters { Self { entry_count, weighted_size, + eviction_count: 0, } } @@ -950,6 +945,12 @@ impl EvictionCounters { let total = &mut self.weighted_size; *total = total.saturating_sub(weight as u64); } + + #[inline] + fn incr_eviction_count(&mut self) { + let count = &mut self.eviction_count; + *count = count.saturating_add(1); + } } #[derive(Default)] @@ -1404,8 +1405,8 @@ where async fn run_pending_tasks( &self, timeout: Option, - max_log_sync_repeats: usize, - eviction_batch_size: usize, + max_log_sync_repeats: u32, + eviction_batch_size: u32, ) -> bool { self.do_run_pending_tasks(timeout, max_log_sync_repeats, eviction_batch_size) .await @@ -1432,8 +1433,8 @@ where async fn do_run_pending_tasks( &self, timeout: Option, - max_log_sync_repeats: usize, - eviction_batch_size: usize, + max_log_sync_repeats: u32, + eviction_batch_size: u32, ) -> bool { if self.max_capacity == Some(0) { return false; @@ -1448,46 +1449,50 @@ where } else { None }; - let mut calls = 0; + let mut should_process_logs = true; + let mut calls = 0u32; let current_ec = self.entry_count.load(); let current_ws = self.weighted_size.load(); let mut eviction_state = EvictionState::new(current_ec, current_ws, self.removal_notifier.as_ref()); loop { - calls += 1; + if should_process_logs { + let r_len = self.read_op_ch.len(); + if r_len > 0 { + self.apply_reads(&mut deqs, &mut timer_wheel, r_len).await; + } - let r_len = self.read_op_ch.len(); - if r_len > 0 { - self.apply_reads(&mut deqs, &mut timer_wheel, r_len).await; - } + let w_len = self.write_op_ch.len(); + if w_len > 0 { + self.apply_writes(&mut deqs, &mut timer_wheel, w_len, &mut eviction_state) + .await; + } - let w_len = self.write_op_ch.len(); - if w_len > 0 { - self.apply_writes(&mut deqs, &mut timer_wheel, w_len, &mut eviction_state) - .await; - } + if self.eviction_policy == EvictionPolicyConfig::TinyLfu + && self.should_enable_frequency_sketch(&eviction_state.counters) + { + self.enable_frequency_sketch(&eviction_state.counters).await; + } - if self.eviction_policy == EvictionPolicyConfig::TinyLfu - && self.should_enable_frequency_sketch(&eviction_state.counters) - { - self.enable_frequency_sketch(&eviction_state.counters).await; - } + // If there are any async tasks waiting in `BaseCache::schedule_write_op` + // method for the write op channel to have enough room, notify them. + let listeners = self.write_op_ch_ready_event.total_listeners(); + if listeners > 0 { + let n = listeners.min(WRITE_LOG_CH_SIZE - self.write_op_ch.len()); + // Notify the `n` listeners. The `notify` method accepts 0, so no + // need to check if `n` is greater than 0. + self.write_op_ch_ready_event.notify(n); + } - // If there are any async tasks waiting in `BaseCache::schedule_write_op` - // method for the write op channel to have enough room, notify them. - let listeners = self.write_op_ch_ready_event.total_listeners(); - if listeners > 0 { - let n = listeners.min(WRITE_LOG_CH_SIZE - self.write_op_ch.len()); - // Notify the `n` listeners. The `notify` method accepts 0, so no - // need to check if `n` is greater than 0. - self.write_op_ch_ready_event.notify(n); + calls += 1; } // Set this flag to `false`. The `evict_*` and `invalidate_*` methods // below may set it to `true` if there are more entries to evict in next // loop. eviction_state.more_entries_to_evict = false; + let last_eviction_count = eviction_state.counters.eviction_count; // Evict entries if there are any expired entries in the hierarchical // timer wheels. @@ -1542,14 +1547,21 @@ where // Check whether to continue this loop or not. - let should_process_logs = calls <= max_log_sync_repeats + should_process_logs = calls <= max_log_sync_repeats && (self.read_op_ch.len() >= READ_LOG_FLUSH_POINT || self.write_op_ch.len() >= WRITE_LOG_FLUSH_POINT); - if !should_process_logs && !eviction_state.more_entries_to_evict { + let should_evict_more_entries = eviction_state.more_entries_to_evict + // Check if there were any entries evicted in this loop. + && (eviction_state.counters.eviction_count - last_eviction_count) > 0; + + // Break the loop if there will be nothing to do in next loop. + if !should_process_logs && !should_evict_more_entries { break; } + // Break the loop if the eviction listener is set and timeout has been + // reached. if let (Some(to), Some(started)) = (timeout, started_at) { let elapsed = self .current_time_from_expiration_clock() @@ -1786,6 +1798,7 @@ where .notify_entry_removal(key, &entry, RemovalCause::Size) .await; } + eviction_state.counters.incr_eviction_count(); } entry.entry_info().set_policy_gen(gen); return; @@ -1833,6 +1846,8 @@ where .notify_entry_removal(vic_key, &vic_entry, RemovalCause::Size) .await; } + eviction_state.counters.incr_eviction_count(); + // And then remove the victim from the deques. Self::handle_remove( deqs, @@ -1889,6 +1904,7 @@ where .notify_entry_removal(key, &entry, RemovalCause::Size) .await; } + eviction_state.counters.incr_eviction_count(); } } } @@ -2172,6 +2188,7 @@ where .notify_entry_removal(key, &entry, RemovalCause::Expired) .await; } + eviction_state.counters.incr_eviction_count(); Self::handle_remove_without_timer_wheel( deqs, entry, @@ -2189,7 +2206,7 @@ where &self, deqs: &mut MutexGuard<'_, Deques>, timer_wheel: &mut TimerWheel, - batch_size: usize, + batch_size: u32, state: &mut EvictionState<'_, K, V>, ) where V: Clone, @@ -2220,7 +2237,7 @@ where cache_region: CacheRegion, deqs: &mut Deques, timer_wheel: &mut TimerWheel, - batch_size: usize, + batch_size: u32, now: Instant, eviction_state: &mut EvictionState<'_, K, V>, ) where @@ -2258,8 +2275,15 @@ where // we change `last_modified` and `last_accessed` in `EntryInfo` from // `Option` to `Instant`. Some((key, hash, true, _) | (key, hash, false, None)) => { + // `is_dirty` is true or `last_modified` is None. Skip this entry + // as it may have been updated by this or other async task but + // its `WriteOp` is not processed yet. let (ao_deq, wo_deq) = deqs.select_mut(cache_region); self.skip_updated_entry_ao(&key, hash, deq_name, ao_deq, wo_deq); + // Set `more_to_evict` to `false` to make `run_pending_tasks` to + // return early. This will help that `schedule_write_op` to send + // the `WriteOp` to the write op channel. + more_to_evict = false; continue; } None => { @@ -2292,6 +2316,7 @@ where .notify_entry_removal(key, &entry, cause) .await; } + eviction_state.counters.incr_eviction_count(); let (ao_deq, wo_deq) = deqs.select_mut(cache_region); Self::handle_remove_with_deques( deq_name, @@ -2304,6 +2329,7 @@ where } else { let (ao_deq, wo_deq) = deqs.select_mut(cache_region); self.skip_updated_entry_ao(&key, hash, deq_name, ao_deq, wo_deq); + more_to_evict = false; } } @@ -2358,7 +2384,7 @@ where &self, deqs: &mut Deques, timer_wheel: &mut TimerWheel, - batch_size: usize, + batch_size: u32, now: Instant, eviction_state: &mut EvictionState<'_, K, V>, ) where @@ -2395,7 +2421,14 @@ where // we change `last_modified` and `last_accessed` in `EntryInfo` from // `Option` to `Instant`. Some((key, hash, true, _) | (key, hash, false, None)) => { + // `is_dirty` is true or `last_modified` is None. Skip this entry + // as it may have been updated by this or other async task but + // its `WriteOp` is not processed yet. self.skip_updated_entry_wo(&key, hash, deqs); + // Set `more_to_evict` to `false` to make `run_pending_tasks` to + // return early. This will help that `schedule_write_op` to send + // the `WriteOp` to the write op channel. + more_to_evict = false; continue; } None => { @@ -2424,9 +2457,11 @@ where .notify_entry_removal(key, &entry, cause) .await; } + eviction_state.counters.incr_eviction_count(); Self::handle_remove(deqs, timer_wheel, entry, None, &mut eviction_state.counters); } else { self.skip_updated_entry_wo(&key, hash, deqs); + more_to_evict = false; } } @@ -2440,7 +2475,7 @@ where invalidator: &Invalidator, deqs: &mut Deques, timer_wheel: &mut TimerWheel, - batch_size: usize, + batch_size: u32, eviction_state: &mut EvictionState<'_, K, V>, ) where V: Clone, @@ -2502,7 +2537,7 @@ where &self, deqs: &mut Deques, timer_wheel: &mut TimerWheel, - batch_size: usize, + batch_size: u32, weights_to_evict: u64, eviction_state: &mut EvictionState<'_, K, V>, ) where @@ -2519,19 +2554,15 @@ where break; } - let maybe_key_hash_ts = deqs - .select_mut(CacheRegion::MainProbation) - .0 - .peek_front() - .map(|node| { - let entry_info = node.element.entry_info(); - ( - Arc::clone(node.element.key()), - node.element.hash(), - entry_info.is_dirty(), - entry_info.last_accessed(), - ) - }); + let maybe_key_hash_ts = deqs.select_mut(CACHE_REGION).0.peek_front().map(|node| { + let entry_info = node.element.entry_info(); + ( + Arc::clone(node.element.key()), + node.element.hash(), + entry_info.is_dirty(), + entry_info.last_accessed(), + ) + }); let (key, hash, ts) = match maybe_key_hash_ts { Some((key, hash, false, Some(ts))) => (key, hash, ts), @@ -2539,8 +2570,15 @@ where // we change `last_modified` and `last_accessed` in `EntryInfo` from // `Option` to `Instant`. Some((key, hash, true, _) | (key, hash, false, None)) => { + // `is_dirty` is true or `last_modified` is None. Skip this entry + // as it may have been updated by this or other async task but + // its `WriteOp` is not processed yet. let (ao_deq, wo_deq) = deqs.select_mut(CACHE_REGION); self.skip_updated_entry_ao(&key, hash, deq_name, ao_deq, wo_deq); + // Set `more_to_evict` to `false` to make `run_pending_tasks` to + // return early. This will help that `schedule_write_op` to send + // the `WriteOp` to the write op channel. + more_to_evict = false; continue; } None => { @@ -2575,6 +2613,7 @@ where .notify_entry_removal(key, &entry, RemovalCause::Size) .await; } + eviction_state.counters.incr_eviction_count(); let weight = entry.policy_weight(); let (deq, write_order_deq) = deqs.select_mut(CacheRegion::MainProbation); Self::handle_remove_with_deques( @@ -2589,6 +2628,7 @@ where } else { let (ao_deq, wo_deq) = deqs.select_mut(CacheRegion::MainProbation); self.skip_updated_entry_ao(&key, hash, deq_name, ao_deq, wo_deq); + more_to_evict = false; } } diff --git a/src/future/cache.rs b/src/future/cache.rs index 812e26fb..5bc2fd64 100644 --- a/src/future/cache.rs +++ b/src/future/cache.rs @@ -5113,8 +5113,8 @@ mod tests { const MAX_CAPACITY: u64 = 20; const EVICTION_TIMEOUT: Duration = Duration::from_millis(30); - const MAX_LOG_SYNC_REPEATS: usize = 1; - const EVICTION_BATCH_SIZE: usize = 1; + const MAX_LOG_SYNC_REPEATS: u32 = 1; + const EVICTION_BATCH_SIZE: u32 = 1; let hk_conf = HousekeeperConfig::new( Some(EVICTION_TIMEOUT), @@ -5176,8 +5176,8 @@ mod tests { const EVICTION_TIMEOUT: Duration = Duration::from_millis(30); const LISTENER_DELAY: Duration = Duration::from_millis(11); - const MAX_LOG_SYNC_REPEATS: usize = 1; - const EVICTION_BATCH_SIZE: usize = 1; + const MAX_LOG_SYNC_REPEATS: u32 = 1; + const EVICTION_BATCH_SIZE: u32 = 1; let hk_conf = HousekeeperConfig::new( Some(EVICTION_TIMEOUT), diff --git a/src/future/housekeeper.rs b/src/future/housekeeper.rs index d20cad31..49cf334d 100644 --- a/src/future/housekeeper.rs +++ b/src/future/housekeeper.rs @@ -24,12 +24,13 @@ use futures_util::future::{BoxFuture, Shared}; #[async_trait] pub(crate) trait InnerSync { - /// Runs the pending tasks. Returns `true` if there are more entries to evict. + /// Runs the pending tasks. Returns `true` if there are more entries to evict in + /// next run. async fn run_pending_tasks( &self, timeout: Option, - max_log_sync_repeats: usize, - eviction_batch_size: usize, + max_log_sync_repeats: u32, + eviction_batch_size: u32, ) -> bool; /// Notifies all the async tasks waiting in `BaseCache::schedule_write_op` method @@ -58,10 +59,10 @@ pub(crate) struct Housekeeper { maintenance_task_timeout: Option, /// The maximum repeat count for receiving operation logs from the read and write /// log channels. Default: `MAX_LOG_SYNC_REPEATS`. - max_log_sync_repeats: usize, + max_log_sync_repeats: u32, /// The batch size of entries to be processed by each internal eviction method. /// Default: `EVICTION_BATCH_SIZE`. - eviction_batch_size: usize, + eviction_batch_size: u32, auto_run_enabled: AtomicBool, #[cfg(test)] pub(crate) start_count: AtomicUsize, @@ -138,12 +139,14 @@ impl Housekeeper { cache.notify_write_op_ch_is_ready(); } - pub(crate) async fn try_run_pending_tasks(&self, cache: Arc) -> bool + /// Tries to run the pending tasks if the lock is free. Returns `true` if there + /// are more entries to evict in next run. + pub(crate) async fn try_run_pending_tasks(&self, cache: &Arc) -> bool where T: InnerSync + Send + Sync + 'static, { if let Some(mut current_task) = self.current_task.try_lock() { - self.do_run_pending_tasks(Arc::clone(&cache), &mut current_task) + self.do_run_pending_tasks(Arc::clone(cache), &mut current_task) .await; } else { return false; diff --git a/src/sync/cache.rs b/src/sync/cache.rs index bd231251..53650850 100644 --- a/src/sync/cache.rs +++ b/src/sync/cache.rs @@ -4803,8 +4803,8 @@ mod tests { const MAX_CAPACITY: u64 = 20; const EVICTION_TIMEOUT: Duration = Duration::from_millis(30); - const MAX_LOG_SYNC_REPEATS: usize = 1; - const EVICTION_BATCH_SIZE: usize = 1; + const MAX_LOG_SYNC_REPEATS: u32 = 1; + const EVICTION_BATCH_SIZE: u32 = 1; let hk_conf = HousekeeperConfig::new( Some(EVICTION_TIMEOUT), @@ -4866,8 +4866,8 @@ mod tests { const EVICTION_TIMEOUT: Duration = Duration::from_millis(30); const LISTENER_DELAY: Duration = Duration::from_millis(11); - const MAX_LOG_SYNC_REPEATS: usize = 1; - const EVICTION_BATCH_SIZE: usize = 1; + const MAX_LOG_SYNC_REPEATS: u32 = 1; + const EVICTION_BATCH_SIZE: u32 = 1; let hk_conf = HousekeeperConfig::new( Some(EVICTION_TIMEOUT), diff --git a/src/sync_base/base_cache.rs b/src/sync_base/base_cache.rs index fe6cc9ab..e6b9ec05 100644 --- a/src/sync_base/base_cache.rs +++ b/src/sync_base/base_cache.rs @@ -790,6 +790,7 @@ impl<'a, K, V> EvictionState<'a, K, V> { struct EvictionCounters { entry_count: u64, weighted_size: u64, + eviction_count: u64, } impl EvictionCounters { @@ -798,6 +799,7 @@ impl EvictionCounters { Self { entry_count, weighted_size, + eviction_count: 0, } } @@ -814,6 +816,12 @@ impl EvictionCounters { let total = &mut self.weighted_size; *total = total.saturating_sub(weight as u64); } + + #[inline] + fn incr_eviction_count(&mut self) { + let count = &mut self.eviction_count; + *count = count.saturating_add(1); + } } #[derive(Default)] @@ -1242,8 +1250,8 @@ where fn run_pending_tasks( &self, timeout: Option, - max_log_sync_repeats: usize, - eviction_batch_size: usize, + max_log_sync_repeats: u32, + eviction_batch_size: u32, ) -> bool { self.do_run_pending_tasks(timeout, max_log_sync_repeats, eviction_batch_size) } @@ -1262,8 +1270,8 @@ where fn do_run_pending_tasks( &self, timeout: Option, - max_log_sync_repeats: usize, - eviction_batch_size: usize, + max_log_sync_repeats: u32, + eviction_batch_size: u32, ) -> bool { if self.max_capacity == Some(0) { return false; @@ -1278,35 +1286,39 @@ where } else { None }; - let mut calls = 0; + let mut should_process_logs = true; + let mut calls = 0u32; let current_ec = self.entry_count.load(); let current_ws = self.weighted_size.load(); let mut eviction_state = EvictionState::new(current_ec, current_ws, self.removal_notifier.as_ref()); loop { - calls += 1; + if should_process_logs { + let r_len = self.read_op_ch.len(); + if r_len > 0 { + self.apply_reads(&mut deqs, &mut timer_wheel, r_len); + } - let r_len = self.read_op_ch.len(); - if r_len > 0 { - self.apply_reads(&mut deqs, &mut timer_wheel, r_len); - } + let w_len = self.write_op_ch.len(); + if w_len > 0 { + self.apply_writes(&mut deqs, &mut timer_wheel, w_len, &mut eviction_state); + } - let w_len = self.write_op_ch.len(); - if w_len > 0 { - self.apply_writes(&mut deqs, &mut timer_wheel, w_len, &mut eviction_state); - } + if self.eviction_policy == EvictionPolicyConfig::TinyLfu + && self.should_enable_frequency_sketch(&eviction_state.counters) + { + self.enable_frequency_sketch(&eviction_state.counters); + } - if self.eviction_policy == EvictionPolicyConfig::TinyLfu - && self.should_enable_frequency_sketch(&eviction_state.counters) - { - self.enable_frequency_sketch(&eviction_state.counters); + calls += 1; } // Set this flag to `false`. The `evict_*` and `invalidate_*` methods // below may set it to `true` if there are more entries to evict in next // loop. eviction_state.more_entries_to_evict = false; + let last_eviction_count = eviction_state.counters.eviction_count; // Evict entries if there are any expired entries in the hierarchical // timer wheels. @@ -1357,14 +1369,21 @@ where // Check whether to continue this loop or not. - let should_process_logs = calls <= max_log_sync_repeats + should_process_logs = calls <= max_log_sync_repeats && (self.read_op_ch.len() >= READ_LOG_FLUSH_POINT || self.write_op_ch.len() >= WRITE_LOG_FLUSH_POINT); - if !should_process_logs && !eviction_state.more_entries_to_evict { + let should_evict_more_entries = eviction_state.more_entries_to_evict + // Check if there were any entries evicted in this loop. + && (eviction_state.counters.eviction_count - last_eviction_count) > 0; + + // Break the loop if there will be nothing to do in next loop. + if !should_process_logs && !should_evict_more_entries { break; } + // Break the loop if the eviction listener is set and timeout has been + // reached. if let (Some(to), Some(started)) = (timeout, started_at) { let elapsed = self .current_time_from_expiration_clock() @@ -1584,6 +1603,7 @@ where let key = Arc::clone(&kh.key); eviction_state.notify_entry_removal(key, &entry, RemovalCause::Size); } + eviction_state.counters.incr_eviction_count(); } entry.entry_info().set_policy_gen(gen); return; @@ -1629,6 +1649,7 @@ where RemovalCause::Size, ); } + eviction_state.counters.incr_eviction_count(); // And then remove the victim from the deques. Self::handle_remove( deqs, @@ -1680,6 +1701,7 @@ where if eviction_state.is_notifier_enabled() { eviction_state.notify_entry_removal(key, &entry, RemovalCause::Size); } + eviction_state.counters.incr_eviction_count(); } } }; @@ -1953,6 +1975,7 @@ where let key = Arc::clone(key); eviction_state.notify_entry_removal(key, &entry, RemovalCause::Expired); } + eviction_state.counters.incr_eviction_count(); Self::handle_remove_without_timer_wheel( deqs, entry, @@ -1971,7 +1994,7 @@ where &self, deqs: &mut Deques, timer_wheel: &mut TimerWheel, - batch_size: usize, + batch_size: u32, state: &mut EvictionState<'_, K, V>, ) where V: Clone, @@ -1998,7 +2021,7 @@ where cache_region: CacheRegion, deqs: &mut Deques, timer_wheel: &mut TimerWheel, - batch_size: usize, + batch_size: u32, now: Instant, eviction_state: &mut EvictionState<'_, K, V>, ) where @@ -2037,7 +2060,14 @@ where // we change `last_modified` and `last_accessed` in `EntryInfo` from // `Option` to `Instant`. Some((key, hash, true, _) | (key, hash, false, None)) => { + // `is_dirty` is true or `last_modified` is None. Skip this entry + // as it may have been updated by this or other async task but + // its `WriteOp` is not processed yet. self.skip_updated_entry_ao(&key, hash, deq_name, ao_deq, wo_deq); + // Set `more_to_evict` to `false` to make `run_pending_tasks` to + // return early. This will help that `schedule_write_op` to send + // the `WriteOp` to the write op channel. + more_to_evict = false; continue; } None => { @@ -2064,6 +2094,7 @@ where if eviction_state.is_notifier_enabled() { eviction_state.notify_entry_removal(key, &entry, cause); } + eviction_state.counters.incr_eviction_count(); Self::handle_remove_with_deques( deq_name, ao_deq, @@ -2074,6 +2105,7 @@ where ); } else { self.skip_updated_entry_ao(&key, hash, deq_name, ao_deq, wo_deq); + more_to_evict = false; } } @@ -2128,7 +2160,7 @@ where &self, deqs: &mut Deques, timer_wheel: &mut TimerWheel, - batch_size: usize, + batch_size: u32, now: Instant, eviction_state: &mut EvictionState<'_, K, V>, ) where @@ -2166,6 +2198,11 @@ where // `Option` to `Instant`. Some((key, hash, true, _) | (key, hash, false, None)) => { self.skip_updated_entry_wo(&key, hash, deqs); + // If the key is the caller key, set `more_to_evict` to `false` + // to make `run_pending_tasks` to return early. This will help + // that `schedule_write_op`, a caller in the call tree, to send + // the `WriteOp` to the write op channel. + more_to_evict = false; continue; } None => { @@ -2188,9 +2225,13 @@ where if eviction_state.is_notifier_enabled() { eviction_state.notify_entry_removal(key, &entry, cause); } + eviction_state.counters.incr_eviction_count(); Self::handle_remove(deqs, timer_wheel, entry, None, &mut eviction_state.counters); } else { self.skip_updated_entry_wo(&key, hash, deqs); + // if caller_key.map(|ck| ck == &key).unwrap_or_default() { + more_to_evict = false; + // } } } @@ -2204,7 +2245,7 @@ where invalidator: &Invalidator, deqs: &mut Deques, timer_wheel: &mut TimerWheel, - batch_size: usize, + batch_size: u32, eviction_state: &mut EvictionState<'_, K, V>, ) where V: Clone, @@ -2265,7 +2306,7 @@ where &self, deqs: &mut Deques, timer_wheel: &mut TimerWheel, - batch_size: usize, + batch_size: u32, weights_to_evict: u64, eviction_state: &mut EvictionState<'_, K, V>, ) where @@ -2299,7 +2340,14 @@ where // `last_modified` and `last_accessed` in `EntryInfo` from `Option` to // `Instant`. Some((key, hash, true, _) | (key, hash, false, None)) => { + // `is_dirty` is true or `last_modified` is None. Skip this entry + // as it may have been updated by this or other async task but + // its `WriteOp` is not processed yet. self.skip_updated_entry_ao(&key, hash, deq_name, ao_deq, wo_deq); + // Set `more_to_evict` to `false` to make `run_pending_tasks` to + // return early. This will help that `schedule_write_op` to send + // the `WriteOp` to the write op channel. + more_to_evict = false; continue; } None => { @@ -2328,6 +2376,7 @@ where if eviction_state.is_notifier_enabled() { eviction_state.notify_entry_removal(key, &entry, RemovalCause::Size); } + eviction_state.counters.incr_eviction_count(); let weight = entry.policy_weight(); Self::handle_remove_with_deques( deq_name, @@ -2340,6 +2389,9 @@ where evicted = evicted.saturating_add(weight as u64); } else { self.skip_updated_entry_ao(&key, hash, deq_name, ao_deq, wo_deq); + // if caller_key.map(|ck| ck == &key).unwrap_or_default() { + more_to_evict = false; + // } } } From dfbbe3f96f3f43a68ad09f74bf63ce679d3d03dc Mon Sep 17 00:00:00 2001 From: Tatsuya Kawano Date: Tue, 16 Apr 2024 18:02:02 +0800 Subject: [PATCH 8/9] Make a single call to `run_pending_tasks` to evict as many entries as possible from the cache Tweak some unit tests. --- src/future/cache.rs | 9 +++++---- src/sync/cache.rs | 9 +++++---- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/future/cache.rs b/src/future/cache.rs index 5bc2fd64..1c17e19c 100644 --- a/src/future/cache.rs +++ b/src/future/cache.rs @@ -5112,11 +5112,12 @@ mod tests { async fn no_batch_size_limit_on_eviction() { const MAX_CAPACITY: u64 = 20; - const EVICTION_TIMEOUT: Duration = Duration::from_millis(30); + const EVICTION_TIMEOUT: Duration = Duration::from_nanos(0); const MAX_LOG_SYNC_REPEATS: u32 = 1; const EVICTION_BATCH_SIZE: u32 = 1; let hk_conf = HousekeeperConfig::new( + // Timeout should be ignored when the eviction listener is not provided. Some(EVICTION_TIMEOUT), Some(MAX_LOG_SYNC_REPEATS), Some(EVICTION_BATCH_SIZE), @@ -5145,7 +5146,7 @@ mod tests { cache.run_pending_tasks().await; assert_eq!(cache.entry_count(), MAX_CAPACITY); - // Insert more items the cache. + // Insert more items to the cache. for i in MAX_CAPACITY..(MAX_CAPACITY * 2) { let v = format!("v{i}"); cache.insert(i, v).await @@ -5163,7 +5164,7 @@ mod tests { cache.run_pending_tasks().await; assert_eq!(cache.entry_count(), MAX_CAPACITY); - // Now the old keys should be gone. + // Now all the old keys should be gone. assert!(!cache.contains_key(&0)); assert!(!cache.contains_key(&(MAX_CAPACITY - 1))); // And the new keys should exist. @@ -5222,7 +5223,7 @@ mod tests { assert_eq!(listener_call_count.load(Ordering::Acquire), 0); assert_eq!(cache.entry_count(), MAX_CAPACITY); - // Insert more items the cache. + // Insert more items to the cache. for i in MAX_CAPACITY..(MAX_CAPACITY * 2) { let v = format!("v{i}"); cache.insert(i, v).await diff --git a/src/sync/cache.rs b/src/sync/cache.rs index 53650850..03a7c3ac 100644 --- a/src/sync/cache.rs +++ b/src/sync/cache.rs @@ -4802,11 +4802,12 @@ mod tests { fn no_batch_size_limit_on_eviction() { const MAX_CAPACITY: u64 = 20; - const EVICTION_TIMEOUT: Duration = Duration::from_millis(30); + const EVICTION_TIMEOUT: Duration = Duration::from_nanos(0); const MAX_LOG_SYNC_REPEATS: u32 = 1; const EVICTION_BATCH_SIZE: u32 = 1; let hk_conf = HousekeeperConfig::new( + // Timeout should be ignored when the eviction listener is not provided. Some(EVICTION_TIMEOUT), Some(MAX_LOG_SYNC_REPEATS), Some(EVICTION_BATCH_SIZE), @@ -4835,7 +4836,7 @@ mod tests { cache.run_pending_tasks(); assert_eq!(cache.entry_count(), MAX_CAPACITY); - // Insert more items the cache. + // Insert more items to the cache. for i in MAX_CAPACITY..(MAX_CAPACITY * 2) { let v = format!("v{i}"); cache.insert(i, v) @@ -4853,7 +4854,7 @@ mod tests { cache.run_pending_tasks(); assert_eq!(cache.entry_count(), MAX_CAPACITY); - // Now the old keys should be gone. + // Now all the old keys should be gone. assert!(!cache.contains_key(&0)); assert!(!cache.contains_key(&(MAX_CAPACITY - 1))); // And the new keys should exist. @@ -4912,7 +4913,7 @@ mod tests { assert_eq!(listener_call_count.load(Ordering::Acquire), 0); assert_eq!(cache.entry_count(), MAX_CAPACITY); - // Insert more items the cache. + // Insert more items to the cache. for i in MAX_CAPACITY..(MAX_CAPACITY * 2) { let v = format!("v{i}"); cache.insert(i, v); From ccb285b22b7bb7a3c1b464d669555e5aff4c0af8 Mon Sep 17 00:00:00 2001 From: Tatsuya Kawano Date: Tue, 16 Apr 2024 19:27:09 +0800 Subject: [PATCH 9/9] Tweak some source code comments --- src/sync_base/base_cache.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/sync_base/base_cache.rs b/src/sync_base/base_cache.rs index e6b9ec05..3d68de2d 100644 --- a/src/sync_base/base_cache.rs +++ b/src/sync_base/base_cache.rs @@ -2198,10 +2198,6 @@ where // `Option` to `Instant`. Some((key, hash, true, _) | (key, hash, false, None)) => { self.skip_updated_entry_wo(&key, hash, deqs); - // If the key is the caller key, set `more_to_evict` to `false` - // to make `run_pending_tasks` to return early. This will help - // that `schedule_write_op`, a caller in the call tree, to send - // the `WriteOp` to the write op channel. more_to_evict = false; continue; } @@ -2229,9 +2225,7 @@ where Self::handle_remove(deqs, timer_wheel, entry, None, &mut eviction_state.counters); } else { self.skip_updated_entry_wo(&key, hash, deqs); - // if caller_key.map(|ck| ck == &key).unwrap_or_default() { more_to_evict = false; - // } } } @@ -2389,9 +2383,7 @@ where evicted = evicted.saturating_add(weight as u64); } else { self.skip_updated_entry_ao(&key, hash, deq_name, ao_deq, wo_deq); - // if caller_key.map(|ck| ck == &key).unwrap_or_default() { more_to_evict = false; - // } } }