Fix some N+1s when calculating notification recipients
What does this MR do?
Fixes a couple of bad N+1s (or worse) in NotificationRecipientService
.
Are there points in the code the reviewer needs to double check?
I don't think so, but here's the commit message:
Fix some N+1s when calculating notification recipients
First N+1: we may have loaded a user's notification settings already, but not have loaded their sources. Because we're iterating through, we'd potentially load sources that are completely unrelated, just because they belong to this user.
Second N+1: we do a separate query for each user who could be subscribed to or unsubcribed from the target. It's actually more efficient in this case to get all subscriptions at once, as we will need to check most of them.
We can fix both by the slightly unpleasant means of checking IDs manually, rather than object equality.
Why was this MR needed?
Try this on a production Rails console:
# paste the contents of https://gitlab.com/gitlab-org/gitlab-ce/blob/master/spec/support/helpers/query_recorder.rb
merge_request = MergeRequest.find(8845120)
user = User.find_by_username('smcgivern')
ActiveRecord::QueryRecorder.new { NotificationRecipientService.build_recipients(merge_request, user, action: "resolve_all_discussions") }.count
# => 2117
# Yes, that's a lot of queries!
class User
def notification_settings_for(source)
if notification_settings.loaded?
notification_settings.find do |notification|
notification.source_type == source.class.base_class.name &&
notification.source_id == source.id
end
else
notification_settings.find_or_initialize_by(source: source)
end
end
end
class NotificationRecipient
def unsubscribed?
return false unless @target
return false unless @target.respond_to?(:subscriptions)
subscription = @target.subscriptions.find { |subscription| subscription.user_id == @user.id }
subscription && !subscription.subscribed
end
end
ActiveRecord::QueryRecorder.new { NotificationRecipientService.build_recipients(merge_request, user, action: "resolve_all_discussions") }.count
# => 526
# This is still a lot, but it's better
Does this MR meet the acceptance criteria?
-
Changelog entry added, if necessary -
Tests added for this feature/bug - Conform by the code review guidelines
-
Has been reviewed by a Backend maintainer
-
What are the relevant issue numbers?
Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/45534.
Merge request reports
Activity
added 1 commit
- 0206476a - Fix some N+1s when calculating notification recipients
mentioned in issue #45534 (closed)
- Resolved by Rémy Coutable
@smcgivern Thank you, looks good to me!
mentioned in commit 5024c4aa
mentioned in issue gitlab-org/release/tasks#262 (closed)
added feature freeze label
mentioned in issue #47005 (closed)
mentioned in issue #48402 (closed)
added devopsplan label