Skip to content
This repository has been archived by the owner on Jun 27, 2019. It is now read-only.

Enhance /deaths, /levels and /timeline timeout by caching call to get member #135

Merged
merged 4 commits into from
Oct 10, 2018

Conversation

Tschis
Copy link
Contributor

@Tschis Tschis commented Oct 9, 2018

Bot searches all entries to get first 100/200 visible entries for the user that executed the command.

Every entry would then execute a call into get_member, and this would take some time, which in turn could make the bot timeout while waiting.

By introducing the cache, the same members will not be searched repeatedly and the performance is increased a lot.

@Tschis Tschis requested a review from Galarzaa90 October 9, 2018 17:48
@Tschis Tschis added the enhancement Improvements, cleanup or optimization. label Oct 9, 2018
@Tschis Tschis changed the title Fix deaths timeout by caching call to get member Enhance /deaths, /levels and /timeline timeout by caching call to get member Oct 9, 2018
@Tschis
Copy link
Contributor Author

Tschis commented Oct 9, 2018

PR modified to also add caching to /levels and /timeline, which suffered from the same bottleneck.

cogs/tibia.py Outdated
@@ -221,6 +225,15 @@ def __init__(self, bot: NabBot):
except CannotPaginate as e:
await ctx.send(e)

@staticmethod
def _get_cached_user_(self, user_id, user_unknown, users_cache, user_servers):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a cog, all commands are listed first in alphabetical order, followed by auxiliary functions, this should be moved accordingly.

cogs/tibia.py Outdated
member_user = self.bot.get_member(user_id, user_servers)
cached_user = user_unknown if member_user is None else member_user
users_cache[user_id] = cached_user
return cached_user
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to have this return Union[User, None] instead of Union[str, User] so the possible values are similar, and in the implementations you don't have to deal with having a variable for "user_unknown" to compare on later.

Instead of using Dict.get(), you can use key in Dict to check if the user is in the cache, if it's not, then fetch.

I know I have committed this sin in the past, but I'm working on it!

@Galarzaa90
Copy link
Owner

Just wanted to add that, other than those minor details, this is a very good solution.

@Galarzaa90
Copy link
Owner

Perfect, thanks!

@Galarzaa90 Galarzaa90 merged commit 9943603 into dev Oct 10, 2018
@Galarzaa90 Galarzaa90 deleted the feat-fix-deaths-timeout branch October 10, 2018 15:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Improvements, cleanup or optimization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants