PERF: Speed up PostSerializer#reactions by avoiding nested loop take 2
#313
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
tgxworld
force-pushed
the
perf_improvement
branch
from
99cc734
to
a81401b
Compare
tgxworld
commented
Oct 11, 2024
| @@ -29,8 +35,7 @@ def posts | |||
| ) | |||
|
|
|||
| posts.each do |post| | |||
| post.post_actions_with_reaction_users = | |||
| post_actions_with_reaction_users.select { |post_action| post_action.post_id == post.id } | |||
There was a problem hiding this comment.
This was looping through the whole array once per post which is inefficient. Instead, we reduce the array into a hash keyed by the post_action.post_id instead so that we can just do a O(1) hash lookup when setting post.post_actions_with_reaction_users.
This is a follow-up to 7467349. Within the `object.post_actions.reject` loop, we are running through another loop given by `object.post_actions_with_reaction_users&.find`. When the `object.post_actions` and `object.post_actions_with_reaction_users` array is large, we end up spending alot of time executing the loops. This commit resolves the problem by reducing the number of records we loop in `object.post_actions_with_reaction_users` by prefiltering the records by `post_action.id` to remove alot of unnecessary iterations in the loop. Local benchmarks for the method for 8000 items in `object.post_actions` records and 2000 items in `post_actions_with_reaction_users` shows the runtime of the method decreasing from roughly 7seconds to about 5ms.
tgxworld
force-pushed
the
perf_improvement
branch
from
a81401b
to
b641675
Compare
tgxworld
commented
Oct 11, 2024
Comment on lines
+115
to
+118
| object.post_actions_with_reaction_users[post_action.id] | ||
| &.reaction_user | ||
| &.reaction | ||
| &.reaction_value == DiscourseReactions::Reaction.main_reaction_id |
There was a problem hiding this comment.
No more nested loop here and we can just do a O(1) hash lookup.
SamSaffron
approved these changes
Oct 11, 2024
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.
This is a follow-up to 7467349.
Within the
object.post_actions.rejectloop, we are running throughanother loop given by
object.post_actions_with_reaction_users&.find.When the
object.post_actionsandobject.post_actions_with_reaction_usersarray is large, we end upspending alot of time executing the loops.
This commit resolves the problem by reducing the number of records we
loop in
object.post_actions_with_reaction_usersby prefiltering therecords by
post_action.idto remove alot of unnecessary iterations inthe loop.
Local benchmarks for the method for 8000 items in
object.post_actionsrecords and 2000items in
post_actions_with_reaction_usersshows the runtime of themethod decreasing from roughly 7seconds to about 5ms.