Skip to content

Commit

Permalink
Get rid of the duplicate users cache (serenity-rs#2662)
Browse files Browse the repository at this point in the history 
This cache was just duplicating information already present in `Guild::members`
and therefore should be removed.

This saves around 700 MBs for my bot (pre-`FixedString`).

This has to refactor `utils::content_safe` to always take a `Guild` instead
of`Cache`, but in practice it was mostly pulling from the guild cache anyway
and this means it is more likely to respect nicknames and other information,
while losing the ability to clean mentions from DMs, which do not matter.
  • Loading branch information
@GnomedDev
GnomedDev committed Mar 19, 2024
1 parent a0776d0 commit f381f1e
Show file tree
Hide file tree
Showing 7 changed files with 76 additions and 295 deletions.
21 changes: 8 additions & 13 deletions examples/e05_command_framework/src/main.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -344,23 +344,18 @@ async fn commands(ctx: &Context, msg: &Message) -> CommandResult {
#[command]
async fn say(ctx: &Context, msg: &Message, mut args: Args) -> CommandResult {
match args.single_quoted::<String>() {
Ok(x) => {
let settings = if let Some(guild_id) = msg.guild_id {
Ok(mut x) => {
// We only need to wipe mentions in guilds, as DM mentions do not matter.
if let Some(guild) = msg.guild(&ctx.cache) {
// By default roles, users, and channel mentions are cleaned.
ContentSafeOptions::default()
let settings = ContentSafeOptions::default()
// We do not want to clean channal mentions as they do not ping users.
.clean_channel(false)
// If it's a guild channel, we want mentioned users to be displayed as their
// display name.
.display_as_member_from(guild_id)
} else {
ContentSafeOptions::default().clean_channel(false).clean_role(false)
};
.clean_channel(false);

let content = content_safe(&ctx.cache, x, &settings, &msg.mentions);

msg.channel_id.say(&ctx.http, &content).await?;
x = content_safe(&guild, x, &settings, &msg.mentions);
}

msg.channel_id.say(&ctx.http, x).await?;
return Ok(());
},
Err(_) => {
Expand Down
31 changes: 2 additions & 29 deletions src/cache/event.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,14 +101,7 @@ impl CacheUpdate for GuildCreateEvent {

fn update(&mut self, cache: &Cache) -> Option<()> {
cache.unavailable_guilds.remove(&self.guild.id);
let mut guild = self.guild.clone();

for (user_id, member) in &mut guild.members {
cache.update_user_entry(&member.user);
if let Some(u) = cache.user(user_id) {
member.user = u.clone();
}
}
let guild = self.guild.clone();

cache.guilds.insert(self.guild.id, guild);
for channel_id in self.guild.channels.keys() {
Expand Down Expand Up @@ -163,15 +156,9 @@ impl CacheUpdate for GuildMemberAddEvent {
type Output = ();

fn update(&mut self, cache: &Cache) -> Option<()> {
let user_id = self.member.user.id;
cache.update_user_entry(&self.member.user);
if let Some(u) = cache.user(user_id) {
self.member.user = u.clone();
}

if let Some(mut guild) = cache.guilds.get_mut(&self.member.guild_id) {
guild.member_count += 1;
guild.members.insert(user_id, self.member.clone());
guild.members.insert(self.member.user.id, self.member.clone());
}

None
Expand All @@ -195,8 +182,6 @@ impl CacheUpdate for GuildMemberUpdateEvent {
type Output = Member;

fn update(&mut self, cache: &Cache) -> Option<Self::Output> {
cache.update_user_entry(&self.user);

if let Some(mut guild) = cache.guilds.get_mut(&self.guild_id) {
let item = if let Some(member) = guild.members.get_mut(&self.user.id) {
let item = Some(member.clone());
Expand Down Expand Up @@ -248,10 +233,6 @@ impl CacheUpdate for GuildMembersChunkEvent {
type Output = ();

fn update(&mut self, cache: &Cache) -> Option<()> {
for member in self.members.values() {
cache.update_user_entry(&member.user);
}

if let Some(mut g) = cache.guilds.get_mut(&self.guild_id) {
g.members.extend(self.members.clone());
}
Expand Down Expand Up @@ -423,14 +404,6 @@ impl CacheUpdate for PresenceUpdateEvent {
type Output = ();

fn update(&mut self, cache: &Cache) -> Option<()> {
if let Some(user) = self.presence.user.to_user() {
cache.update_user_entry(&user);
}

if let Some(user) = cache.user(self.presence.user.id) {
self.presence.user.update_with_user(&user);
}

if let Some(guild_id) = self.presence.guild_id {
if let Some(mut guild) = cache.guilds.get_mut(&guild_id) {
// If the member went offline, remove them from the presence list.
Expand Down
81 changes: 0 additions & 81 deletions src/cache/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ use std::sync::Arc;
#[cfg(feature = "temp_cache")]
use std::time::Duration;

use dashmap::mapref::entry::Entry;
use dashmap::mapref::one::{MappedRef, Ref};
use dashmap::DashMap;
#[cfg(feature = "temp_cache")]
Expand Down Expand Up @@ -186,22 +185,6 @@ pub struct Cache {
/// are "sent in" over time through the receiving of [`Event::GuildCreate`]s.
pub(crate) unavailable_guilds: MaybeMap<GuildId, ()>,

// Users cache:
// ---
/// A map of users that the current user sees.
///
/// Users are added to - and updated from - this map via the following received events:
///
/// - [`GuildMemberAdd`][`GuildMemberAddEvent`]
/// - [`GuildMemberRemove`][`GuildMemberRemoveEvent`]
/// - [`GuildMembersChunk`][`GuildMembersChunkEvent`]
/// - [`PresenceUpdate`][`PresenceUpdateEvent`]
/// - [`Ready`][`ReadyEvent`]
///
/// Note, however, that users are _not_ removed from the map on removal events such as
/// [`GuildMemberRemove`][`GuildMemberRemoveEvent`], as other structs such as members or
/// recipients may still exist.
pub(crate) users: MaybeMap<UserId, User>,
/// A map of users' presences. This is updated in real-time. Note that status updates are often
/// "eaten" by the gateway, and this should not be treated as being entirely 100% accurate.
pub(crate) presences: MaybeMap<UserId, Presence>,
Expand Down Expand Up @@ -275,7 +258,6 @@ impl Cache {
guilds: MaybeMap(settings.cache_guilds.then(DashMap::default)),
unavailable_guilds: MaybeMap(settings.cache_guilds.then(DashMap::default)),

users: MaybeMap(settings.cache_users.then(DashMap::default)),
presences: MaybeMap(settings.cache_users.then(DashMap::default)),

messages: DashMap::default(),
Expand Down Expand Up @@ -643,56 +625,6 @@ impl Cache {
self.settings.write().max_messages = max;
}

/// Retrieves a [`User`] from the cache's [`Self::users`] map, if it exists.
///
/// The only advantage of this method is that you can pass in anything that is indirectly a
/// [`UserId`].
///
/// # Examples
///
/// Retrieve a user from the cache and print their name:
///
/// ```rust,no_run
/// # use serenity::client::Context;
/// #
/// # async fn test(context: &Context) -> Result<(), Box<dyn std::error::Error>> {
/// if let Some(user) = context.cache.user(7) {
/// println!("User with Id 7 is currently named {}", user.name);
/// }
/// # Ok(())
/// # }
/// ```
#[inline]
pub fn user<U: Into<UserId>>(&self, user_id: U) -> Option<UserRef<'_>> {
self._user(user_id.into())
}

#[cfg(feature = "temp_cache")]
fn _user(&self, user_id: UserId) -> Option<UserRef<'_>> {
if let Some(user) = self.users.get(&user_id) {
Some(CacheRef::from_ref(user))
} else {
self.temp_users.get(&user_id).map(CacheRef::from_arc)
}
}

#[cfg(not(feature = "temp_cache"))]
fn _user(&self, user_id: UserId) -> Option<UserRef<'_>> {
self.users.get(&user_id).map(CacheRef::from_ref)
}

/// Clones all users and returns them.
#[inline]
pub fn users(&self) -> ReadOnlyMapRef<'_, UserId, User> {
self.users.as_read_only()
}

/// Returns the amount of cached users.
#[inline]
pub fn user_count(&self) -> usize {
self.users.len()
}

/// This method provides a reference to the user used by the bot.
#[inline]
pub fn current_user(&self) -> CurrentUserRef<'_> {
Expand Down Expand Up @@ -745,19 +677,6 @@ impl Cache {
pub fn update<E: CacheUpdate>(&self, e: &mut E) -> Option<E::Output> {
e.update(self)
}

pub(crate) fn update_user_entry(&self, user: &User) {
if let Some(users) = &self.users.0 {
match users.entry(user.id) {
Entry::Vacant(e) => {
e.insert(user.clone());
},
Entry::Occupied(mut e) => {
e.get_mut().clone_from(user);
},
}
}
}
}

impl Default for Cache {
Expand Down
14 changes: 0 additions & 14 deletions src/model/gateway.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -293,20 +293,6 @@ impl PresenceUser {
pub fn to_user(&self) -> Option<User> {
self.clone().into_user()
}

#[cfg(feature = "cache")] // method is only used with the cache feature enabled
pub(crate) fn update_with_user(&mut self, user: &User) {
self.id = user.id;
if let Some(avatar) = user.avatar {
self.avatar = Some(avatar);
}
self.bot = Some(user.bot);
self.discriminator = user.discriminator;
self.name = Some(user.name.clone());
if let Some(public_flags) = user.public_flags {
self.public_flags = Some(public_flags);
}
}
}

/// Information detailing the current online status of a [`User`].
Expand Down
17 changes: 4 additions & 13 deletions src/model/user.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ use serde::{Deserialize, Serialize};
use super::prelude::*;
#[cfg(feature = "model")]
use crate::builder::{Builder, CreateMessage, EditProfile};
#[cfg(all(feature = "cache", feature = "model"))]
use crate::cache::{Cache, UserRef};
#[cfg(feature = "collector")]
use crate::collector::{MessageCollector, ReactionCollector};
#[cfg(feature = "collector")]
Expand Down Expand Up @@ -672,13 +670,6 @@ impl UserId {
cache_http.http().create_private_channel(&map).await
}

/// Attempts to find a [`User`] by its Id in the cache.
#[cfg(feature = "cache")]
#[inline]
pub fn to_user_cached(self, cache: &impl AsRef<Cache>) -> Option<UserRef<'_>> {
cache.as_ref().user(self)
}

/// First attempts to find a [`User`] by its Id in the cache, upon failure requests it via the
/// REST API.
///
Expand All @@ -695,18 +686,18 @@ impl UserId {
/// May also return an [`Error::Json`] if there is an error in deserializing the user.
#[inline]
pub async fn to_user(self, cache_http: impl CacheHttp) -> Result<User> {
#[cfg(feature = "cache")]
#[cfg(feature = "temp_cache")]
{
if let Some(cache) = cache_http.cache() {
if let Some(user) = cache.user(self) {
return Ok(user.clone());
if let Some(user) = cache.temp_users.get(&self) {
return Ok(User::clone(&user));
}
}
}

let user = cache_http.http().get_user(self).await?;

#[cfg(all(feature = "cache", feature = "temp_cache"))]
#[cfg(feature = "temp_cache")]
{
if let Some(cache) = cache_http.cache() {
use crate::cache::MaybeOwnedArc;
Expand Down
38 changes: 1 addition & 37 deletions src/utils/argument_convert/user.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,36 +23,6 @@ impl fmt::Display for UserParseError {
}
}

#[cfg(feature = "cache")]
fn lookup_by_global_cache(ctx: impl CacheHttp, s: &str) -> Option<User> {
let users = &ctx.cache()?.users;

let lookup_by_id = || users.get(&s.parse().ok()?).map(|u| u.clone());

let lookup_by_mention = || users.get(&crate::utils::parse_user_mention(s)?).map(|u| u.clone());

let lookup_by_name_and_discrim = || {
let (name, discrim) = crate::utils::parse_user_tag(s)?;
users.iter().find_map(|m| {
let user = m.value();
(user.discriminator == discrim && user.name.eq_ignore_ascii_case(name))
.then(|| user.clone())
})
};

let lookup_by_name = || {
users.iter().find_map(|m| {
let user = m.value();
(&*user.name == s).then(|| user.clone())
})
};

lookup_by_id()
.or_else(lookup_by_mention)
.or_else(lookup_by_name_and_discrim)
.or_else(lookup_by_name)
}

/// Look up a user by a string case-insensitively.
///
/// Requires the cache feature to be enabled. If a user is not in cache, they will not be found!
Expand All @@ -72,13 +42,7 @@ impl ArgumentConvert for User {
channel_id: Option<ChannelId>,
s: &str,
) -> Result<Self, Self::Err> {
// Try to look up in global user cache via a variety of methods
#[cfg(feature = "cache")]
if let Some(user) = lookup_by_global_cache(&ctx, s) {
return Ok(user);
}

// If not successful, convert as a Member which uses HTTP endpoints instead of cache
// Convert as a Member which uses HTTP endpoints instead of cache
if let Ok(member) = Member::convert(&ctx, guild_id, channel_id, s).await {
return Ok(member.user);
}
Expand Down
Loading

0 comments on commit f381f1e

Please sign in to comment.