From f5cc9dc88d18989100b2cc26e1be942c1d1f22a2 Mon Sep 17 00:00:00 2001 From: Tatsuya Kawano Date: Mon, 14 Aug 2023 20:17:00 +0800 Subject: [PATCH 1/3] Do not copy internal pointers on updating an entry Move internal pointers to deque nodes from `ValueEntry` to `EntryInfo`, so that the pointers are no longer copied on updating an entry. --- src/common/concurrent.rs | 68 ++++++++--------------------- src/common/concurrent/entry_info.rs | 57 +++++++++++++++++++++--- src/dash/base_cache.rs | 11 ++--- src/sync_base/base_cache.rs | 13 +++--- 4 files changed, 80 insertions(+), 69 deletions(-) diff --git a/src/common/concurrent.rs b/src/common/concurrent.rs index 3da5b898..9e3774be 100644 --- a/src/common/concurrent.rs +++ b/src/common/concurrent.rs @@ -1,6 +1,5 @@ use crate::common::{deque::DeqNode, time::Instant}; -use parking_lot::Mutex; use std::{ptr::NonNull, sync::Arc}; use tagptr::TagNonNull; use triomphe::Arc as TrioArc; @@ -62,11 +61,11 @@ impl Clone for KeyHash { pub(crate) struct KeyDate { key: Arc, - entry_info: TrioArc, + entry_info: TrioArc>, } impl KeyDate { - pub(crate) fn new(key: Arc, entry_info: &TrioArc) -> Self { + pub(crate) fn new(key: Arc, entry_info: &TrioArc>) -> Self { Self { key, entry_info: TrioArc::clone(entry_info), @@ -91,11 +90,11 @@ impl KeyDate { pub(crate) struct KeyHashDate { key: Arc, hash: u64, - entry_info: TrioArc, + entry_info: TrioArc>, } impl KeyHashDate { - pub(crate) fn new(kh: KeyHash, entry_info: &TrioArc) -> Self { + pub(crate) fn new(kh: KeyHash, entry_info: &TrioArc>) -> Self { Self { key: kh.key, hash: kh.hash, @@ -111,7 +110,7 @@ impl KeyHashDate { self.hash } - pub(crate) fn entry_info(&self) -> &EntryInfo { + pub(crate) fn entry_info(&self) -> &EntryInfo { &self.entry_info } } @@ -172,59 +171,28 @@ impl AccessTime for DeqNode> { } // DeqNode for an access order queue. -type KeyDeqNodeAo = TagNonNull>, 2>; +pub(crate) type KeyDeqNodeAo = TagNonNull>, 2>; // DeqNode for the write order queue. -type KeyDeqNodeWo = NonNull>>; - -pub(crate) struct DeqNodes { - access_order_q_node: Option>, - write_order_q_node: Option>, -} - -// We need this `unsafe impl` as DeqNodes have NonNull pointers. -unsafe impl Send for DeqNodes {} +pub(crate) type KeyDeqNodeWo = NonNull>>; pub(crate) struct ValueEntry { pub(crate) value: V, - info: TrioArc, - nodes: Mutex>, + info: TrioArc>, } impl ValueEntry { - pub(crate) fn new(value: V, entry_info: TrioArc) -> Self { - #[cfg(feature = "unstable-debug-counters")] - self::debug_counters::InternalGlobalDebugCounters::value_entry_created(); - - Self { - value, - info: entry_info, - nodes: Mutex::new(DeqNodes { - access_order_q_node: None, - write_order_q_node: None, - }), - } - } - - pub(crate) fn new_from(value: V, entry_info: TrioArc, other: &Self) -> Self { + pub(crate) fn new(value: V, entry_info: TrioArc>) -> Self { #[cfg(feature = "unstable-debug-counters")] self::debug_counters::InternalGlobalDebugCounters::value_entry_created(); - let nodes = { - let other_nodes = other.nodes.lock(); - DeqNodes { - access_order_q_node: other_nodes.access_order_q_node, - write_order_q_node: other_nodes.write_order_q_node, - } - }; Self { value, info: entry_info, - nodes: Mutex::new(nodes), } } - pub(crate) fn entry_info(&self) -> &TrioArc { + pub(crate) fn entry_info(&self) -> &TrioArc> { &self.info } @@ -250,33 +218,31 @@ impl ValueEntry { } pub(crate) fn access_order_q_node(&self) -> Option> { - self.nodes.lock().access_order_q_node + self.info.access_order_q_node() } pub(crate) fn set_access_order_q_node(&self, node: Option>) { - self.nodes.lock().access_order_q_node = node; + self.info.set_access_order_q_node(node); } pub(crate) fn take_access_order_q_node(&self) -> Option> { - self.nodes.lock().access_order_q_node.take() + self.info.take_access_order_q_node() } pub(crate) fn write_order_q_node(&self) -> Option> { - self.nodes.lock().write_order_q_node + self.info.write_order_q_node() } pub(crate) fn set_write_order_q_node(&self, node: Option>) { - self.nodes.lock().write_order_q_node = node; + self.info.set_write_order_q_node(node); } pub(crate) fn take_write_order_q_node(&self) -> Option> { - self.nodes.lock().write_order_q_node.take() + self.info.take_write_order_q_node() } pub(crate) fn unset_q_nodes(&self) { - let mut nodes = self.nodes.lock(); - nodes.access_order_q_node = None; - nodes.write_order_q_node = None; + self.info.unset_q_nodes(); } } diff --git a/src/common/concurrent/entry_info.rs b/src/common/concurrent/entry_info.rs index 82efc71e..556059dd 100644 --- a/src/common/concurrent/entry_info.rs +++ b/src/common/concurrent/entry_info.rs @@ -1,9 +1,19 @@ use std::sync::atomic::{AtomicBool, AtomicU32, Ordering}; -use super::AccessTime; +use super::{AccessTime, KeyDeqNodeAo, KeyDeqNodeWo}; use crate::common::{concurrent::atomic_time::AtomicInstant, time::Instant}; -pub(crate) struct EntryInfo { +use parking_lot::Mutex; + +pub(crate) struct DeqNodes { + access_order_q_node: Option>, + write_order_q_node: Option>, +} + +// We need this `unsafe impl` as DeqNodes have NonNull pointers. +unsafe impl Send for DeqNodes {} + +pub(crate) struct EntryInfo { /// `is_admitted` indicates that the entry has been admitted to the /// cache. When `false`, it means the entry is _temporary_ admitted to /// the cache or evicted from the cache (so it should not have LRU nodes). @@ -15,9 +25,10 @@ pub(crate) struct EntryInfo { last_accessed: AtomicInstant, last_modified: AtomicInstant, policy_weight: AtomicU32, + nodes: Mutex>, } -impl EntryInfo { +impl EntryInfo { #[inline] pub(crate) fn new(timestamp: Instant, policy_weight: u32) -> Self { #[cfg(feature = "unstable-debug-counters")] @@ -29,6 +40,10 @@ impl EntryInfo { last_accessed: AtomicInstant::new(timestamp), last_modified: AtomicInstant::new(timestamp), policy_weight: AtomicU32::new(policy_weight), + nodes: Mutex::new(DeqNodes { + access_order_q_node: None, + write_order_q_node: None, + }), } } @@ -60,16 +75,46 @@ impl EntryInfo { pub(crate) fn set_policy_weight(&self, size: u32) { self.policy_weight.store(size, Ordering::Release); } + + pub(crate) fn access_order_q_node(&self) -> Option> { + self.nodes.lock().access_order_q_node + } + + pub(crate) fn set_access_order_q_node(&self, node: Option>) { + self.nodes.lock().access_order_q_node = node; + } + + pub(crate) fn take_access_order_q_node(&self) -> Option> { + self.nodes.lock().access_order_q_node.take() + } + + pub(crate) fn write_order_q_node(&self) -> Option> { + self.nodes.lock().write_order_q_node + } + + pub(crate) fn set_write_order_q_node(&self, node: Option>) { + self.nodes.lock().write_order_q_node = node; + } + + pub(crate) fn take_write_order_q_node(&self) -> Option> { + self.nodes.lock().write_order_q_node.take() + } + + pub(crate) fn unset_q_nodes(&self) { + let mut nodes = self.nodes.lock(); + nodes.access_order_q_node = None; + nodes.write_order_q_node = None; + } } #[cfg(feature = "unstable-debug-counters")] -impl Drop for EntryInfo { +impl Drop for EntryInfo { fn drop(&mut self) { super::debug_counters::InternalGlobalDebugCounters::entry_info_dropped(); } } -impl AccessTime for EntryInfo { +impl AccessTime for EntryInfo { #[inline] fn last_accessed(&self) -> Option { self.last_accessed.instant() @@ -152,7 +197,7 @@ mod test { } if let Some(size) = expected { - assert_eq!(size_of::(), size); + assert_eq!(size_of::>(), size); } else { panic!("No expected size for {:?} with Rust version {}", arch, ver); } diff --git a/src/dash/base_cache.rs b/src/dash/base_cache.rs index a90ac63c..72b987b4 100644 --- a/src/dash/base_cache.rs +++ b/src/dash/base_cache.rs @@ -275,9 +275,10 @@ where // Update .and_modify(|entry| { // NOTES on `new_value_entry_from` method: - // 1. The internal EntryInfo will be shared between the old and new ValueEntries. - // 2. This method will set the last_accessed and last_modified to the max value to - // prevent this new ValueEntry from being evicted by an expiration policy. + // 1. The internal EntryInfo will be shared between the old and new + // ValueEntries. + // 2. This method will set the dirty flag to prevent this new + // ValueEntry from being evicted by an expiration policy. // 3. This method will update the policy_weight with the new weight. let old_weight = entry.policy_weight(); *entry = self.new_value_entry_from(value.clone(), ts, weight, entry); @@ -333,7 +334,7 @@ where info.set_last_accessed(timestamp); info.set_last_modified(timestamp); info.set_policy_weight(policy_weight); - TrioArc::new(ValueEntry::new_from(value, info, other)) + TrioArc::new(ValueEntry::new(value, info)) } #[inline] @@ -1158,7 +1159,7 @@ where if let Some((_k, entry)) = maybe_entry { Self::handle_remove(deqs, entry, counters); } else if let Some(entry) = self.cache.get(key) { - if entry.last_modified().is_none() { + if entry.is_dirty() { deqs.move_to_back_ao(&entry); deqs.move_to_back_wo(&entry); } else { diff --git a/src/sync_base/base_cache.rs b/src/sync_base/base_cache.rs index 0584f25f..09b97184 100644 --- a/src/sync_base/base_cache.rs +++ b/src/sync_base/base_cache.rs @@ -475,9 +475,10 @@ where // on_modify |_k, old_entry| { // NOTES on `new_value_entry_from` method: - // 1. The internal EntryInfo will be shared between the old and new ValueEntries. - // 2. This method will set the last_accessed and last_modified to the max value to - // prevent this new ValueEntry from being evicted by an expiration policy. + // 1. The internal EntryInfo will be shared between the old and new + // ValueEntries. + // 2. This method will set the dirty flag to prevent this new + // ValueEntry from being evicted by an expiration policy. // 3. This method will update the policy_weight with the new weight. let old_weight = old_entry.policy_weight(); let old_timestamps = (old_entry.last_accessed(), old_entry.last_modified()); @@ -501,7 +502,6 @@ where match (op1, op2) { (Some((_cnt, ins_op)), None) => (ins_op, ts), (None, Some((_cnt, old_entry, (old_last_accessed, old_last_modified), upd_op))) => { - old_entry.unset_q_nodes(); if self.is_removal_notifier_enabled() { self.inner .notify_upsert(key, &old_entry, old_last_accessed, old_last_modified); @@ -516,7 +516,6 @@ where if cnt1 > cnt2 { (ins_op, ts) } else { - old_entry.unset_q_nodes(); if self.is_removal_notifier_enabled() { self.inner.notify_upsert( key, @@ -559,7 +558,7 @@ where info.set_last_accessed(timestamp); info.set_last_modified(timestamp); info.set_policy_weight(policy_weight); - TrioArc::new(ValueEntry::new_from(value, info, other)) + TrioArc::new(ValueEntry::new(value, info)) } #[inline] @@ -1733,7 +1732,7 @@ where } Self::handle_remove(deqs, entry, &mut eviction_state.counters); } else if let Some(entry) = self.cache.get(hash, |k| k == key) { - if entry.last_modified().is_none() { + if entry.is_dirty() { deqs.move_to_back_ao(&entry); deqs.move_to_back_wo(&entry); } else { From 3bd04c02b15b109120eb06714364cfadc2a36b64 Mon Sep 17 00:00:00 2001 From: Tatsuya Kawano Date: Tue, 15 Aug 2023 21:45:37 +0800 Subject: [PATCH 2/3] Do not copy internal pointers on updating an entry Adjust the size of `EntryInfo` in a test. --- src/common/concurrent/entry_info.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/common/concurrent/entry_info.rs b/src/common/concurrent/entry_info.rs index 556059dd..900c2c90 100644 --- a/src/common/concurrent/entry_info.rs +++ b/src/common/concurrent/entry_info.rs @@ -180,12 +180,12 @@ mod test { }; let expected_sizes = match (arch, is_quanta_enabled) { - (Linux64, true) => vec![("1.51", 24)], - (Linux32, true) => vec![("1.51", 24)], - (MacOS64, true) => vec![("1.62", 24)], - (Linux64, false) => vec![("1.66", 56), ("1.51", 72)], - (Linux32, false) => vec![("1.66", 56), ("1.62", 72), ("1.51", 40)], - (MacOS64, false) => vec![("1.62", 56)], + (Linux64, true) => vec![("1.60", 48)], + (Linux32, true) => vec![("1.60", 48)], + (MacOS64, true) => vec![("1.62", 48)], + (Linux64, false) => vec![("1.66", 80), ("1.60", 96)], + (Linux32, false) => vec![("1.66", 72)], + (MacOS64, false) => vec![("1.62", 80)], }; let mut expected = None; From e3282dc3cd4776d9d763a546fe91dd026c832bb1 Mon Sep 17 00:00:00 2001 From: Tatsuya Kawano Date: Tue, 15 Aug 2023 22:12:44 +0800 Subject: [PATCH 3/3] Do not copy internal pointers on updating an entry Adjust the size of `EntryInfo` in a test. --- src/common/concurrent/entry_info.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/common/concurrent/entry_info.rs b/src/common/concurrent/entry_info.rs index 900c2c90..5018aaf2 100644 --- a/src/common/concurrent/entry_info.rs +++ b/src/common/concurrent/entry_info.rs @@ -181,7 +181,7 @@ mod test { let expected_sizes = match (arch, is_quanta_enabled) { (Linux64, true) => vec![("1.60", 48)], - (Linux32, true) => vec![("1.60", 48)], + (Linux32, true) => vec![("1.60", 40)], (MacOS64, true) => vec![("1.62", 48)], (Linux64, false) => vec![("1.66", 80), ("1.60", 96)], (Linux32, false) => vec![("1.66", 72)],