-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Several small perf improvements for realms with large numbers of users #392
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This fixes a performance issue looking up UserProfile objects for realms with a large number of users in the case that a UserProfile object is not in the cache. Thanks to @dbiollo for the suggestion!
Automated message from Dropbox CLA bot @timabbott, it looks like you've already signed the Dropbox CLA. Thanks! |
Since we frequently do filters for only active users, these indexes may help performance in some cases.
This fixes a performance issue joining a server with a large number of users. Thanks to @dbiollo for the suggestion!
This makes it possible to use DevAuthBackend when doing performance/scalability testing on Zulip with many thousands of users. It's unlikely that anyone testing this backend will find it valuable to have more than 100 login buttons on the same page, and if they do, they can always just change this limit. Thanks to @dbiollo for the suggestion!
Just doing the database query is more readable, and has about the same performance as before in the case where active user dicts for the realm are in cache (and is substantially better in the rare case that this isn't in the cache). Thanks to @dbiollo for the perf investigation and suggestion!
This removes from our cache a moderate amount of totally useless alert word data corresponding to users who don't have any alert words. Thanks to @dbiollo for the suggestion!
The browser registers for events via loading the home view, not this interface, and this functionality is available via the API-format register route anyway.
(The test failure is a flake and should be fixed by #394 ) |
@timabbott - I tested the tweaks with my large dataset and it works well as expected! Thanks for implementing the optimizations. |
lgtm fftp |
Merged, thanks for the review! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Thanks @dbiollo for these suggestions (see #375)!