Skip to content

Fix some N+1s when calculating notification recipients

Sean McGivern requested to merge n-plus-one-notification-recipients into master

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?

What are the relevant issue numbers?

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/45534.

Merge request reports