From e3c9e2834e2b496a3a8f55310f87019341677d14 Mon Sep 17 00:00:00 2001 From: William Casarin Date: Fri, 6 Dec 2024 14:36:39 -0800 Subject: [PATCH] cache: refactor cache structure and reduce allocations This commit reworks the cache data structures and operations to significantly cut down on unnecessary allocations and improve overall cache management logic. Key changes include: - Replacing event-specific CacheEntry types with a generic CacheEntry that stores values and timestamps. - Removing the old Arc references in HashMap entries, reducing reference-counted pointer overhead. - Providing helper methods (new(), maybe(), empty(), value()) on CacheEntry for more ergonomic and explicit value handling. - Simplifying the retrieval functions (get_mute_list, get_relay_list, get_contact_list) by using a shared helper (get_cache_entry) that checks for expiration and removes stale entries automatically. - Streamlining add_event and related cache insertion methods to insert the correct CacheEntry variant depending on the event kind. - Removing unnecessary methods (e.g., references_pubkey) and simplifying extensions in nostr_event_extensions.rs. - Enhancing code clarity by separating expiration logic from retrieval and insertion, making the cache easier to maintain, debug, and extend. These improvements should reduce runtime memory overhead and increase code clarity, while maintaining the existing external behavior of the cache. Future changes will be easier to implement, as the new generic CacheEntry abstraction and simplified code paths provide a more maintainable foundation. Signed-off-by: William Casarin --- src/notification_manager/nostr_event_cache.rs | 161 +++++++----------- .../nostr_event_extensions.rs | 10 +- .../nostr_network_helper.rs | 14 +- 3 files changed, 74 insertions(+), 111 deletions(-) diff --git a/src/notification_manager/nostr_event_cache.rs b/src/notification_manager/nostr_event_cache.rs index 6a6d9c4..23cd9ab 100644 --- a/src/notification_manager/nostr_event_cache.rs +++ b/src/notification_manager/nostr_event_cache.rs @@ -2,36 +2,75 @@ use crate::{notification_manager::nostr_event_extensions::MaybeConvertibleToTime use tokio::time::Duration; use nostr_sdk::prelude::*; use std::collections::HashMap; -use std::sync::Arc; use log; use super::nostr_event_extensions::{MaybeConvertibleToRelayList, RelayList, TimestampedMuteList}; -struct CacheEntry { - event: Option, // `None` means the event does not exist as far as we know (It does NOT mean expired) +struct CacheEntry { + value: Option, // `None` means the event does not exist as far as we know (It does NOT mean expired) added_at: nostr::Timestamp, } -impl CacheEntry { +impl CacheEntry { fn is_expired(&self, max_age: Duration) -> bool { let time_delta = TimeDelta::subtracting(nostr::Timestamp::now(), self.added_at); time_delta.negative || (time_delta.delta_abs_seconds > max_age.as_secs()) } } +impl CacheEntry { + pub fn new(value: T) -> Self { + let added_at = nostr::Timestamp::now(); + CacheEntry { value: Some(value), added_at } + } + + pub fn maybe(value: Option) -> Self { + let added_at = nostr::Timestamp::now(); + CacheEntry { value, added_at } + } + + pub fn empty() -> Self { + let added_at = nostr::Timestamp::now(); + CacheEntry { value: None, added_at } + } + + pub fn value(&self) -> Option<&T> { + self.value.as_ref() + } +} + pub struct Cache { - entries: HashMap>, - mute_lists: HashMap>, - contact_lists: HashMap>, - relay_lists: HashMap>, + //entries: HashMap, + mute_lists: HashMap>, + contact_lists: HashMap>, + relay_lists: HashMap>, max_age: Duration, } +fn get_cache_entry(list: &mut HashMap>, pubkey: &PublicKey, max_age: Duration, name: &str) -> Result, CacheError> { + let res = if let Some(entry) = list.get(pubkey) { + if !entry.is_expired(max_age) { + Ok(entry.value().cloned()) + } else { + log::debug!("{} list for pubkey {} is expired, removing it from the cache", name, pubkey.to_hex()); + Err(CacheError::Expired) + } + } else { + Err(CacheError::NotFound) + }; + + if let Err(CacheError::Expired) = &res { + list.remove(pubkey); + } + + res +} + impl Cache { // MARK: - Initialization pub fn new(max_age: Duration) -> Self { Cache { - entries: HashMap::new(), + //entries: HashMap::new(), mute_lists: HashMap::new(), contact_lists: HashMap::new(), relay_lists: HashMap::new(), @@ -41,68 +80,52 @@ impl Cache { // MARK: - Adding items to the cache - pub fn add_optional_mute_list_with_author(&mut self, author: &PublicKey, mute_list: Option) { + pub fn add_optional_mute_list_with_author<'a>(&'a mut self, author: &PublicKey, mute_list: Option<&Event>) { if let Some(mute_list) = mute_list { self.add_event(mute_list); } else { self.mute_lists.insert( - author.clone(), - Arc::new(CacheEntry { - event: None, - added_at: nostr::Timestamp::now(), - }), + author.to_owned(), + CacheEntry::empty(), ); } } - pub fn add_optional_relay_list_with_author(&mut self, author: &PublicKey, relay_list_event: Option) { + pub fn add_optional_relay_list_with_author<'a>(&'a mut self, author: &PublicKey, relay_list_event: Option<&Event>) { if let Some(relay_list_event) = relay_list_event { self.add_event(relay_list_event); } else { self.relay_lists.insert( - author.clone(), - Arc::new(CacheEntry { - event: None, - added_at: nostr::Timestamp::now(), - }), + author.to_owned(), + CacheEntry::empty(), ); } } - pub fn add_optional_contact_list_with_author(&mut self, author: &PublicKey, contact_list: Option) { + pub fn add_optional_contact_list_with_author<'a>(&'a mut self, author: &PublicKey, contact_list: Option<&Event>) { if let Some(contact_list) = contact_list { self.add_event(contact_list); } else { self.contact_lists.insert( - author.clone(), - Arc::new(CacheEntry { - event: None, - added_at: nostr::Timestamp::now(), - }), + author.to_owned(), + CacheEntry::empty(), ); } } - pub fn add_event(&mut self, event: Event) { - let entry = Arc::new(CacheEntry { - event: Some(event.clone()), - added_at: nostr::Timestamp::now(), - }); - self.entries.insert(event.id.clone(), entry.clone()); - + pub fn add_event(&mut self, event: &Event) { match event.kind { Kind::MuteList => { - self.mute_lists.insert(event.pubkey.clone(), entry.clone()); + self.mute_lists.insert(event.pubkey.clone(), CacheEntry::maybe(event.to_timestamped_mute_list())); log::debug!("Added mute list to the cache. Event ID: {}", event.id.to_hex()); } Kind::ContactList => { - self.contact_lists - .insert(event.pubkey.clone(), entry.clone()); log::debug!("Added contact list to the cache. Event ID: {}", event.id.to_hex()); + self.contact_lists.insert(event.pubkey.clone(), CacheEntry::new(event.to_owned())); }, Kind::RelayList => { - self.relay_lists.insert(event.pubkey.clone(), entry.clone()); log::debug!("Added relay list to the cache. Event ID: {}", event.id.to_hex()); + self.relay_lists.insert(event.pubkey.clone(), CacheEntry::maybe(event.to_relay_list())); }, _ => { log::debug!("Unknown event kind, not adding to any cache. Event ID: {}", event.id.to_hex()); @@ -113,70 +136,15 @@ impl Cache { // MARK: - Fetching items from the cache pub fn get_mute_list(&mut self, pubkey: &PublicKey) -> Result, CacheError> { - if let Some(entry) = self.mute_lists.get(pubkey) { - let entry = entry.clone(); // Clone the Arc to avoid borrowing issues - if !entry.is_expired(self.max_age) { - match &entry.event { - Some(event) => { - log::debug!("Cached mute list for pubkey {} was found", pubkey.to_hex()); - return Ok(event.to_timestamped_mute_list()); - } - None => { - log::debug!("Empty mute list cache entry for pubkey {}", pubkey.to_hex()); - return Ok(None); - } - } - } else { - log::debug!("Mute list for pubkey {} is expired, removing it from the cache", pubkey.to_hex()); - self.mute_lists.remove(pubkey); - self.remove_event_from_all_maps(&entry.event); - } - } - log::debug!("Mute list for pubkey {} not found on cache", pubkey.to_hex()); - Err(CacheError::NotFound) + get_cache_entry(&mut self.mute_lists, pubkey, self.max_age, "Mute") } pub fn get_relay_list(&mut self, pubkey: &PublicKey) -> Result, CacheError> { - if let Some(entry) = self.relay_lists.get(pubkey) { - let entry = entry.clone(); // Clone the Arc to avoid borrowing issues - if !entry.is_expired(self.max_age) { - if let Some(event) = entry.event.clone() { - return Ok(event.to_relay_list()); - } - } else { - log::debug!("Relay list for pubkey {} is expired, removing it from the cache", pubkey.to_hex()); - self.mute_lists.remove(pubkey); - self.remove_event_from_all_maps(&entry.event); - } - } - Err(CacheError::NotFound) + get_cache_entry(&mut self.relay_lists, pubkey, self.max_age, "Relay") } pub fn get_contact_list(&mut self, pubkey: &PublicKey) -> Result, CacheError> { - if let Some(entry) = self.contact_lists.get(pubkey) { - let entry = entry.clone(); // Clone the Arc to avoid borrowing issues - if !entry.is_expired(self.max_age) { - return Ok(entry.event.clone()); - } else { - log::debug!("Contact list for pubkey {} is expired, removing it from the cache", pubkey.to_hex()); - self.contact_lists.remove(pubkey); - self.remove_event_from_all_maps(&entry.event); - } - } - Err(CacheError::NotFound) - } - - // MARK: - Removing items from the cache - - fn remove_event_from_all_maps(&mut self, event: &Option) { - if let Some(event) = event { - let event_id = event.id.clone(); - let pubkey = event.pubkey.clone(); - self.entries.remove(&event_id); - self.mute_lists.remove(&pubkey); - self.contact_lists.remove(&pubkey); - } - // We can't remove an event from all maps if the event does not exist + get_cache_entry(&mut self.contact_lists, pubkey, self.max_age, "Contact") } } @@ -184,4 +152,5 @@ impl Cache { #[derive(Debug)] pub enum CacheError { NotFound, + Expired, } diff --git a/src/notification_manager/nostr_event_extensions.rs b/src/notification_manager/nostr_event_extensions.rs index f3117eb..50b3f52 100644 --- a/src/notification_manager/nostr_event_extensions.rs +++ b/src/notification_manager/nostr_event_extensions.rs @@ -3,9 +3,6 @@ use nostr_sdk::{EventId, Kind, TagKind}; /// Temporary scaffolding of old methods that have not been ported to use native Event methods pub trait ExtendedEvent { - /// Checks if the note references a given pubkey - fn references_pubkey(&self, pubkey: &PublicKey) -> bool; - /// Retrieves a set of pubkeys referenced by the note fn referenced_pubkeys(&self) -> std::collections::HashSet; @@ -21,11 +18,6 @@ pub trait ExtendedEvent { // This is a wrapper around the Event type from strfry-policies, which adds some useful methods impl ExtendedEvent for nostr::Event { - /// Checks if the note references a given pubkey - fn references_pubkey(&self, pubkey: &PublicKey) -> bool { - self.referenced_pubkeys().contains(pubkey) - } - /// Retrieves a set of pubkeys referenced by the note fn referenced_pubkeys(&self) -> std::collections::HashSet { self.get_tags_content(SingleLetter(SingleLetterTag::lowercase(Alphabet::P))) @@ -149,6 +141,7 @@ impl MaybeConvertibleToRelayList for nostr::Event { let extracted_relay_list_owned = extracted_relay_list.into_iter() .map(|(url, metadata)| (url.clone(), metadata.as_ref().map(|m| m.clone()))) .collect(); + Some(extracted_relay_list_owned) } } @@ -216,6 +209,7 @@ impl Codable for MuteList { } } +#[derive(Clone)] pub struct TimestampedMuteList { pub mute_list: MuteList, pub timestamp: nostr::Timestamp, diff --git a/src/notification_manager/nostr_network_helper.rs b/src/notification_manager/nostr_network_helper.rs index 2ef1f99..446a034 100644 --- a/src/notification_manager/nostr_network_helper.rs +++ b/src/notification_manager/nostr_network_helper.rs @@ -1,5 +1,5 @@ use tokio::sync::Mutex; -use super::nostr_event_extensions::{MaybeConvertibleToRelayList, MaybeConvertibleToTimestampedMuteList, RelayList, TimestampedMuteList}; +use super::nostr_event_extensions::{RelayList, TimestampedMuteList}; use super::notification_manager::EventSaver; use super::ExtendedEvent; use nostr_sdk::prelude::*; @@ -59,8 +59,8 @@ impl NostrNetworkHelper { // We don't have an answer from the cache, so we need to fetch it let mute_list_event = self.fetch_single_event(pubkey, Kind::MuteList).await; let mut cache_mutex_guard = self.cache.lock().await; - cache_mutex_guard.add_optional_mute_list_with_author(pubkey, mute_list_event.clone()); - Some(mute_list_event?.to_timestamped_mute_list()?) + cache_mutex_guard.add_optional_mute_list_with_author(pubkey, mute_list_event.as_ref()); + cache_mutex_guard.get_mute_list(pubkey).ok()? } pub async fn get_relay_list(&self, pubkey: &PublicKey) -> Option { @@ -74,8 +74,8 @@ impl NostrNetworkHelper { // We don't have an answer from the cache, so we need to fetch it let relay_list_event = NostrNetworkHelper::fetch_single_event_from_client(pubkey, Kind::RelayList, &self.bootstrap_client).await; let mut cache_mutex_guard = self.cache.lock().await; - cache_mutex_guard.add_optional_relay_list_with_author(pubkey, relay_list_event.clone()); - relay_list_event?.to_relay_list() + cache_mutex_guard.add_optional_relay_list_with_author(pubkey, relay_list_event.as_ref()); + cache_mutex_guard.get_relay_list(pubkey).ok()? } pub async fn get_contact_list(&self, pubkey: &PublicKey) -> Option { @@ -89,8 +89,8 @@ impl NostrNetworkHelper { // We don't have an answer from the cache, so we need to fetch it let contact_list_event = self.fetch_single_event(pubkey, Kind::ContactList).await; let mut cache_mutex_guard = self.cache.lock().await; - cache_mutex_guard.add_optional_contact_list_with_author(pubkey, contact_list_event.clone()); - contact_list_event + cache_mutex_guard.add_optional_contact_list_with_author(pubkey, contact_list_event.as_ref()); + cache_mutex_guard.get_contact_list(pubkey).ok()? } // MARK: - Lower level fetching functions