From 26074af3601f506b4b69aee09d1f290cf08d84ac Mon Sep 17 00:00:00 2001 From: Gnome! Date: Sun, 19 Nov 2023 19:26:29 +0000 Subject: [PATCH] Remove instances of cloning in the `Cache` that aren't absolutely needed (#2606) --- src/cache/mod.rs | 143 +++++++++----------------- src/client/dispatch.rs | 5 +- src/model/channel/channel_id.rs | 2 +- src/model/guild/guild_id.rs | 2 +- src/model/guild/mod.rs | 4 +- src/model/guild/partial_guild.rs | 4 +- src/utils/argument_convert/message.rs | 2 +- 7 files changed, 56 insertions(+), 106 deletions(-) diff --git a/src/cache/mod.rs b/src/cache/mod.rs index f2f44530b39..ccfbfa96b28 100644 --- a/src/cache/mod.rs +++ b/src/cache/mod.rs @@ -3,16 +3,13 @@ //! Using the cache allows to avoid REST API requests via the [`http`] module where possible. //! Issuing too many requests will lead to ratelimits. //! -//! Following a policy to never hand out locks, the cache will clone all values when calling its -//! methods. -//! //! # Use by Models //! //! Most models of Discord objects, such as the [`Message`], [`GuildChannel`], or [`Emoji`], have -//! methods for interacting with that single instance. This feature is only compiled if the -//! `methods` feature is enabled. An example of this is [`Guild::edit`], which performs a check to -//! ensure that the current user is the owner of the guild, prior to actually performing the HTTP -//! request. The cache is involved due to the function's use of unlocking the cache and retrieving +//! methods for interacting with that single instance. This feature is only compiled if the `model` +//! feature is enabled. An example of this is [`Guild::edit`], which performs a check to ensure that +//! the current user is the owner of the guild, prior to actually performing the HTTP request. +//! The cache is involved due to the function's use of unlocking the cache and retrieving //! the Id of the current user, and comparing it to the Id of the user that owns the guild. This is //! an inexpensive method of being able to access data required by these sugary methods. //! @@ -109,11 +106,19 @@ impl std::ops::Deref for CacheRef<'_, K, V, T> { } } +type MappedGuildRef<'a, T> = CacheRef<'a, GuildId, T, Guild>; + pub type UserRef<'a> = CacheRef<'a, UserId, User>; +pub type MemberRef<'a> = MappedGuildRef<'a, Member>; pub type GuildRef<'a> = CacheRef<'a, GuildId, Guild>; +pub type GuildRoleRef<'a> = MappedGuildRef<'a, Role>; +pub type SettingsRef<'a> = CacheRef<'a, (), Settings>; pub type CurrentUserRef<'a> = CacheRef<'a, (), CurrentUser>; -pub type GuildChannelRef<'a> = CacheRef<'a, GuildId, GuildChannel, Guild>; +pub type GuildChannelRef<'a> = MappedGuildRef<'a, GuildChannel>; +pub type GuildRolesRef<'a> = MappedGuildRef<'a, HashMap>; +pub type GuildChannelsRef<'a> = MappedGuildRef<'a, HashMap>; pub type ChannelMessagesRef<'a> = CacheRef<'a, ChannelId, HashMap>; +pub type MessageRef<'a> = CacheRef<'a, ChannelId, Message, HashMap>; #[derive(Debug)] pub(crate) struct CachedShardData { @@ -446,9 +451,6 @@ impl Cache { /// Retrieves a [`Guild`]'s member from the cache based on the guild's and user's given Ids. /// - /// **Note**: This will clone the entire member. Instead, retrieve the guild and retrieve from - /// the guild's [`members`] map to avoid this. - /// /// # Examples /// /// Retrieving the member object of the user that posted a message, in a @@ -461,7 +463,7 @@ impl Cache { /// # /// # async fn run(http: Http, cache: Cache, message: Message) { /// # - /// let member = { + /// let roles_len = { /// let channel = match cache.channel(message.channel_id) { /// Some(channel) => channel, /// None => { @@ -472,20 +474,17 @@ impl Cache { /// }, /// }; /// - /// match cache.member(channel.guild_id, message.author.id) { - /// Some(member) => member, - /// None => { - /// if let Err(why) = message.channel_id.say(&http, "Error finding member data").await { - /// println!("Error sending message: {:?}", why); - /// } - /// return; - /// }, - /// } + /// cache.member(channel.guild_id, message.author.id).map(|m| m.roles.len()) /// }; /// - /// let msg = format!("You have {} roles", member.roles.len()); + /// let message_res = if let Some(roles_len) = roles_len { + /// let msg = format!("You have {} roles", roles_len); + /// message.channel_id.say(&http, &msg).await + /// } else { + /// message.channel_id.say(&http, "Error finding member data").await + /// }; /// - /// if let Err(why) = message.channel_id.say(&http, &msg).await { + /// if let Err(why) = message_res { /// println!("Error sending message: {:?}", why); /// } /// # } @@ -494,71 +493,27 @@ impl Cache { /// [`EventHandler::message`]: crate::client::EventHandler::message /// [`members`]: crate::model::guild::Guild::members #[inline] - pub fn member(&self, guild_id: G, user_id: U) -> Option - where - G: Into, - U: Into, - { - self._member(guild_id.into(), user_id.into()) - } - - fn _member(&self, guild_id: GuildId, user_id: UserId) -> Option { - match self.guilds.get(&guild_id) { - Some(guild) => guild.members.get(&user_id).cloned(), - None => None, - } - } - - /// This method allows to only clone a field of a member instead of the entire member by - /// providing a `field_selector`-closure picking what you want to clone. - /// - /// ```rust,no_run - /// # use serenity::cache::Cache; - /// # - /// # fn run() -> Result<(), Box> { - /// # let cache = Cache::default(); - /// // We clone only the `name` instead of the entire channel. - /// if let Some(Some(nick)) = cache.member_field(7, 8, |member| member.nick.clone()) { - /// println!("Member's nick: {}", nick); - /// } - /// # Ok(()) - /// # } - /// ``` - #[inline] - pub fn member_field( + pub fn member( &self, guild_id: impl Into, user_id: impl Into, - field_selector: Fun, - ) -> Option - where - Fun: FnOnce(&Member) -> Ret, - { - self._member_field(guild_id.into(), user_id.into(), field_selector) + ) -> Option> { + self._member(guild_id.into(), user_id.into()) } - fn _member_field( - &self, - guild_id: GuildId, - user_id: UserId, - field_selector: Fun, - ) -> Option - where - Fun: FnOnce(&Member) -> Ret, - { - let guild = self.guilds.get(&guild_id)?; - let member = guild.members.get(&user_id)?; - - Some(field_selector(member)) + fn _member(&self, guild_id: GuildId, user_id: UserId) -> Option> { + let member = self.guilds.get(&guild_id)?.try_map(|g| g.members.get(&user_id)).ok()?; + Some(CacheRef::from_mapped_ref(member)) } #[inline] - pub fn guild_roles(&self, guild_id: impl Into) -> Option> { + pub fn guild_roles(&self, guild_id: impl Into) -> Option> { self._guild_roles(guild_id.into()) } - fn _guild_roles(&self, guild_id: GuildId) -> Option> { - self.guilds.get(&guild_id).map(|g| g.roles.clone()) + fn _guild_roles(&self, guild_id: GuildId) -> Option> { + let roles = self.guilds.get(&guild_id)?.map(|g| &g.roles); + Some(CacheRef::from_mapped_ref(roles)) } /// This method clones and returns all unavailable guilds. @@ -569,18 +524,13 @@ impl Cache { /// This method returns all channels from a guild of with the given `guild_id`. #[inline] - pub fn guild_channels( - &self, - guild_id: impl Into, - ) -> Option> { + pub fn guild_channels(&self, guild_id: impl Into) -> Option> { self._guild_channels(guild_id.into()) } - fn _guild_channels( - &self, - guild_id: GuildId, - ) -> Option> { - self.guilds.get(&guild_id).map(|g| g.channels.clone().into_iter().collect()) + fn _guild_channels(&self, guild_id: GuildId) -> Option> { + let channels = self.guilds.get(&guild_id)?.map(|g| &g.channels); + Some(CacheRef::from_mapped_ref(channels)) } /// Returns the number of guild channels in the cache. @@ -618,7 +568,7 @@ impl Cache { /// /// [`EventHandler::message`]: crate::client::EventHandler::message #[inline] - pub fn message(&self, channel_id: C, message_id: M) -> Option + pub fn message(&self, channel_id: C, message_id: M) -> Option> where C: Into, M: Into, @@ -626,8 +576,10 @@ impl Cache { self._message(channel_id.into(), message_id.into()) } - fn _message(&self, channel_id: ChannelId, message_id: MessageId) -> Option { - self.messages.get(&channel_id).and_then(|messages| messages.get(&message_id).cloned()) + fn _message(&self, channel_id: ChannelId, message_id: MessageId) -> Option> { + let channel_messages = self.messages.get(&channel_id)?; + let message = channel_messages.try_map(|messages| messages.get(&message_id)).ok()?; + Some(CacheRef::from_mapped_ref(message)) } /// Retrieves a [`Guild`]'s role by their Ids. @@ -646,13 +598,13 @@ impl Cache { /// // assuming the cache is in scope, e.g. via `Context` /// if let Some(role) = cache.role(7, 77) { /// println!("Role with Id 77 is called {}", role.name); - /// } + /// }; /// ``` /// /// [`Guild`]: crate::model::guild::Guild /// [`roles`]: crate::model::guild::Guild::roles #[inline] - pub fn role(&self, guild_id: G, role_id: R) -> Option + pub fn role(&self, guild_id: G, role_id: R) -> Option> where G: Into, R: Into, @@ -660,8 +612,9 @@ impl Cache { self._role(guild_id.into(), role_id.into()) } - fn _role(&self, guild_id: GuildId, role_id: RoleId) -> Option { - self.guilds.get(&guild_id).and_then(|g| g.roles.get(&role_id).cloned()) + fn _role(&self, guild_id: GuildId, role_id: RoleId) -> Option> { + let role = self.guilds.get(&guild_id)?.try_map(|g| g.roles.get(&role_id)).ok()?; + Some(CacheRef::from_mapped_ref(role)) } /// Returns the settings. @@ -678,8 +631,8 @@ impl Cache { /// println!("Max settings: {}", cache.settings().max_messages); /// # } /// ``` - pub fn settings(&self) -> Settings { - self.settings.read().clone() + pub fn settings(&self) -> SettingsRef<'_> { + CacheRef::from_guard(self.settings.read()) } /// Sets the maximum amount of messages per channel to cache. diff --git a/src/client/dispatch.rs b/src/client/dispatch.rs index 1becccfa870..55e0f6e9bff 100644 --- a/src/client/dispatch.rs +++ b/src/client/dispatch.rs @@ -224,7 +224,8 @@ fn update_cache_with_event(ctx: Context, event: Event) -> Option<(FullEvent, Opt }, Event::GuildMemberUpdate(mut event) => { let before = if_cache!(update_cache(&ctx, &mut event)); - let after: Option = if_cache!(ctx.cache.member(event.guild_id, event.user.id)); + let after: Option = + if_cache!(ctx.cache.member(event.guild_id, event.user.id).map(|m| m.clone())); FullEvent::GuildMemberUpdate { ctx, @@ -316,7 +317,7 @@ fn update_cache_with_event(ctx: Context, event: Event) -> Option<(FullEvent, Opt }, Event::MessageUpdate(mut event) => { let before = if_cache!(update_cache(&ctx, &mut event)); - let after = if_cache!(ctx.cache.message(event.channel_id, event.id)); + let after = if_cache!(ctx.cache.message(event.channel_id, event.id).map(|m| m.clone())); FullEvent::MessageUpdate { ctx, diff --git a/src/model/channel/channel_id.rs b/src/model/channel/channel_id.rs index cf56f22b59a..512006ab8ff 100644 --- a/src/model/channel/channel_id.rs +++ b/src/model/channel/channel_id.rs @@ -452,7 +452,7 @@ impl ChannelId { #[cfg(feature = "cache")] if let Some(cache) = cache_http.cache() { if let Some(message) = cache.message(self, message_id) { - return Ok(message); + return Ok(message.clone()); } } diff --git a/src/model/guild/guild_id.rs b/src/model/guild/guild_id.rs index 59428d6735b..df1dd806f40 100644 --- a/src/model/guild/guild_id.rs +++ b/src/model/guild/guild_id.rs @@ -1080,7 +1080,7 @@ impl GuildId { { if let Some(cache) = cache_http.cache() { if let Some(member) = cache.member(self, user_id) { - return Ok(member); + return Ok(member.clone()); } } } diff --git a/src/model/guild/mod.rs b/src/model/guild/mod.rs index b5f39951d04..c55ba704da3 100644 --- a/src/model/guild/mod.rs +++ b/src/model/guild/mod.rs @@ -439,9 +439,7 @@ impl Guild { let name = name.as_ref(); let guild_channels = cache.as_ref().guild_channels(self.id)?; - for channel_entry in &guild_channels { - let (id, channel) = channel_entry.pair(); - + for (id, channel) in guild_channels.iter() { if channel.name == name { return Some(*id); } diff --git a/src/model/guild/partial_guild.rs b/src/model/guild/partial_guild.rs index 1eb1441fbe7..633575b052f 100644 --- a/src/model/guild/partial_guild.rs +++ b/src/model/guild/partial_guild.rs @@ -388,9 +388,7 @@ impl PartialGuild { let name = name.as_ref(); let guild_channels = cache.as_ref().guild_channels(self.id)?; - for channel_entry in &guild_channels { - let (id, channel) = channel_entry.pair(); - + for (id, channel) in guild_channels.iter() { if channel.name == name { return Some(*id); } diff --git a/src/utils/argument_convert/message.rs b/src/utils/argument_convert/message.rs index fc86eeb3ee6..646141d3385 100644 --- a/src/utils/argument_convert/message.rs +++ b/src/utils/argument_convert/message.rs @@ -71,7 +71,7 @@ impl ArgumentConvert for Message { #[cfg(feature = "cache")] if let Some(cache) = ctx.cache() { if let Some(msg) = cache.message(channel_id, message_id) { - return Ok(msg); + return Ok(msg.clone()); } }