Fix performance on instances list in admin UI (#15282)

- Reduce duplicate queries
- Remove n+1 queries
- Add accounts count to detailed view
- Add separate action log entry for updating existing domain blocks

app/controllers/admin/domain_blocks_controller.rb

@@ -29,6 +29,7 @@ module Admin
           @domain_block = existing_domain_block
           @domain_block.update(resource_params)
         end
+
         if @domain_block.save
           DomainBlockWorker.perform_async(@domain_block.id)
           log_action :create, @domain_block
@@ -40,7 +41,7 @@ module Admin
     end
 
     def update
-      authorize :domain_block, :create?
+      authorize :domain_block, :update?
 
       @domain_block.update(update_params)
 
@@ -48,7 +49,7 @@ module Admin
 
       if @domain_block.save
         DomainBlockWorker.perform_async(@domain_block.id, severity_changed)
-        log_action :create, @domain_block
+        log_action :update, @domain_block
         redirect_to admin_instances_path(limited: '1'), notice: I18n.t('admin.domain_blocks.created_msg')
       else
         render :edit

app/controllers/admin/instances_controller.rb

@@ -2,65 +2,31 @@
 
 module Admin
   class InstancesController < BaseController
-    before_action :set_domain_block, only: :show
-    before_action :set_domain_allow, only: :show
+    before_action :set_instances, only: :index
     before_action :set_instance, only: :show
 
     def index
       authorize :instance, :index?
-
-      @instances = ordered_instances
     end
 
     def show
       authorize :instance, :show?
-
-      @following_count = Follow.where(account: Account.where(domain: params[:id])).count
-      @followers_count = Follow.where(target_account: Account.where(domain: params[:id])).count
-      @reports_count   = Report.where(target_account: Account.where(domain: params[:id])).count
-      @blocks_count    = Block.where(target_account: Account.where(domain: params[:id])).count
-      @available       = DeliveryFailureTracker.available?(params[:id])
-      @media_storage   = MediaAttachment.where(account: Account.where(domain: params[:id])).sum(:file_file_size)
-      @private_comment = @domain_block&.private_comment
-      @public_comment  = @domain_block&.public_comment
     end
 
     private
 
-    def set_domain_block
-      @domain_block = DomainBlock.rule_for(params[:id])
-    end
-
-    def set_domain_allow
-      @domain_allow = DomainAllow.rule_for(params[:id])
-    end
-
     def set_instance
-      resource   = Account.by_domain_accounts.find_by(domain: params[:id])
-      resource ||= @domain_block
-      resource ||= @domain_allow
+      @instance = Instance.find(params[:id])
+    end
 
-      if resource
-        @instance = Instance.new(resource)
-      else
-        not_found
-      end
+    def set_instances
+      @instances = filtered_instances.page(params[:page])
     end
 
     def filtered_instances
       InstanceFilter.new(whitelist_mode? ? { allowed: true } : filter_params).results
     end
 
-    def paginated_instances
-      filtered_instances.page(params[:page])
-    end
-
-    helper_method :paginated_instances
-
-    def ordered_instances
-      paginated_instances.map { |resource| Instance.new(resource) }
-    end
-
     def filter_params
       params.slice(*InstanceFilter::KEYS).permit(*InstanceFilter::KEYS)
     end

app/controllers/api/v1/instances/peers_controller.rb

@@ -8,7 +8,7 @@ class Api::V1::Instances::PeersController < Api::BaseController
 
   def index
     expires_in 1.day, public: true
-    render_with_cache(expires_in: 1.day) { Account.remote.domains }
+    render_with_cache(expires_in: 1.day) { Instance.where.not(domain: DomainBlock.select(:domain)).pluck(:domain) }
   end
 
   private

app/models/account.rb

@@ -67,6 +67,7 @@ class Account < ApplicationRecord
   include Paginable
   include AccountCounters
   include DomainNormalizable
+  include DomainMaterializable
   include AccountMerging
 
   TRUST_LEVELS = {
@@ -103,7 +104,6 @@ class Account < ApplicationRecord
   scope :bots, -> { where(actor_type: %w(Application Service)) }
   scope :groups, -> { where(actor_type: 'Group') }
   scope :alphabetic, -> { order(domain: :asc, username: :asc) }
-  scope :by_domain_accounts, -> { group(:domain).select(:domain, 'COUNT(*) AS accounts_count').order('accounts_count desc') }
   scope :matches_username, ->(value) { where(arel_table[:username].matches("#{value}%")) }
   scope :matches_display_name, ->(value) { where(arel_table[:display_name].matches("#{value}%")) }
   scope :matches_domain, ->(value) { where(arel_table[:domain].matches("%#{value}%")) }
@@ -438,10 +438,6 @@ class Account < ApplicationRecord
       super - %w(statuses_count following_count followers_count)
     end
 
-    def domains
-      reorder(nil).pluck(Arel.sql('distinct accounts.domain'))
-    end
-
     def inboxes
       urls = reorder(nil).where(protocol: :activitypub).group(:preferred_inbox_url).pluck(Arel.sql("coalesce(nullif(accounts.shared_inbox_url, ''), accounts.inbox_url) AS preferred_inbox_url"))
       DeliveryFailureTracker.without_unavailable(urls)

app/models/concerns/domain_materializable.rb

@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+module DomainMaterializable
+  extend ActiveSupport::Concern
+
+  included do
+    after_create_commit :refresh_instances_view
+  end
+
+  def refresh_instances_view
+    Instance.refresh unless domain.nil? || Instance.where(domain: domain).exists?
+  end
+end

app/models/domain_allow.rb

@@ -12,6 +12,7 @@
 
 class DomainAllow < ApplicationRecord
   include DomainNormalizable
+  include DomainMaterializable
 
   validates :domain, presence: true, uniqueness: true, domain: true
 

app/models/domain_block.rb

@@ -16,6 +16,7 @@
 
 class DomainBlock < ApplicationRecord
   include DomainNormalizable
+  include DomainMaterializable
 
   enum severity: [:silence, :suspend, :noop]
 

app/models/instance.rb

@@ -1,26 +1,63 @@
 # frozen_string_literal: true
+# == Schema Information
+#
+# Table name: instances
+#
+#  domain         :string           primary key
+#  accounts_count :bigint(8)
+#
 
-class Instance
-  include ActiveModel::Model
+class Instance < ApplicationRecord
+  self.primary_key = :domain
 
-  attr_accessor :domain, :accounts_count, :domain_block
+  has_many :accounts, foreign_key: :domain, primary_key: :domain
 
-  def initialize(resource)
-    @domain         = resource.domain
-    @accounts_count = resource.respond_to?(:accounts_count) ? resource.accounts_count : nil
-    @domain_block   = resource.is_a?(DomainBlock) ? resource : DomainBlock.rule_for(domain)
-    @domain_allow   = resource.is_a?(DomainAllow) ? resource : DomainAllow.rule_for(domain)
+  belongs_to :domain_block, foreign_key: :domain, primary_key: :domain
+  belongs_to :domain_allow, foreign_key: :domain, primary_key: :domain
+
+  scope :matches_domain, ->(value) { where(arel_table[:domain].matches("%#{value}%")) }
+
+  def self.refresh
+    Scenic.database.refresh_materialized_view(table_name, concurrently: true, cascade: false)
   end
 
-  def countable?
-    @accounts_count.present?
+  def readonly?
+    true
+  end
+
+  def delivery_failure_tracker
+    @delivery_failure_tracker ||= DeliveryFailureTracker.new(domain)
+  end
+
+  def following_count
+    @following_count ||= Follow.where(account: accounts).count
+  end
+
+  def followers_count
+    @followers_count ||= Follow.where(target_account: accounts).count
+  end
+
+  def reports_count
+    @reports_count ||= Report.where(target_account: accounts).count
+  end
+
+  def blocks_count
+    @blocks_count ||= Block.where(target_account: accounts).count
+  end
+
+  def public_comment
+    domain_block&.public_comment
+  end
+
+  def private_comment
+    domain_block&.private_comment
+  end
+
+  def media_storage
+    @media_storage ||= MediaAttachment.where(account: accounts).sum(:file_file_size)
   end
 
   def to_param
     domain
   end
-
-  def cache_key
-    domain
-  end
 end

app/models/instance_filter.rb

@@ -13,18 +13,27 @@ class InstanceFilter
   end
 
   def results
-    if params[:limited].present?
-      scope = DomainBlock
-      scope = scope.matches_domain(params[:by_domain]) if params[:by_domain].present?
-      scope.order(id: :desc)
-    elsif params[:allowed].present?
-      scope = DomainAllow
-      scope = scope.matches_domain(params[:by_domain]) if params[:by_domain].present?
-      scope.order(id: :desc)
+    scope = Instance.includes(:domain_block, :domain_allow).order(accounts_count: :desc)
+
+    params.each do |key, value|
+      scope.merge!(scope_for(key, value.to_s.strip)) if value.present?
+    end
+
+    scope
+  end
+
+  private
+
+  def scope_for(key, value)
+    case key.to_s
+    when 'limited'
+      Instance.joins(:domain_block).reorder(Arel.sql('domain_blocks.id desc'))
+    when 'allowed'
+      Instance.joins(:domain_allow).reorder(Arel.sql('domain_allows.id desc'))
+    when 'by_domain'
+      Instance.matches_domain(value)
     else
-      scope = Account.remote
-      scope = scope.matches_domain(params[:by_domain]) if params[:by_domain].present?
-      scope.by_domain_accounts
+      raise "Unknown filter: #{key}"
     end
   end
 end

app/models/unavailable_domain.rb

@@ -12,6 +12,8 @@
 class UnavailableDomain < ApplicationRecord
   include DomainNormalizable
 
+  validates :domain, presence: true, uniqueness: true
+
   after_commit :reset_cache!
 
   private

app/policies/domain_block_policy.rb

@@ -13,6 +13,10 @@ class DomainBlockPolicy < ApplicationPolicy
     admin?
   end
 
+  def update?
+    admin?
+  end
+
   def destroy?
     admin?
   end

app/presenters/instance_presenter.rb

@@ -29,7 +29,7 @@ class InstancePresenter
   end
 
   def domain_count
-    Rails.cache.fetch('distinct_domain_count') { Account.distinct.count(:domain) }
+    Rails.cache.fetch('distinct_domain_count') { Instance.count }
   end
 
   def sample_accounts

app/views/admin/instances/_instance.html.haml

@@ -0,0 +1,25 @@
+.directory__tag
+  = link_to admin_instance_path(instance) do
+    %h4
+      = instance.domain
+      %small
+        - if instance.domain_block
+          - first_item = true
+          - if !instance.domain_block.noop?
+            = t("admin.domain_blocks.severity.#{instance.domain_block.severity}")
+            - first_item = false
+          - unless instance.domain_block.suspend?
+            - if instance.domain_block.reject_media?
+              - unless first_item
+                •
+              = t('admin.domain_blocks.rejecting_media')
+              - first_item = false
+            - if instance.domain_block.reject_reports?
+              - unless first_item
+                •
+              = t('admin.domain_blocks.rejecting_reports')
+        - elsif whitelist_mode?
+          = t('admin.accounts.whitelisted')
+        - else
+          = t('admin.accounts.no_limits_imposed')
+    .trends__item__current{ title: t('admin.instances.known_accounts', count: instance.accounts_count) }= number_to_human instance.accounts_count, strip_insignificant_zeros: true

app/views/admin/instances/index.html.haml

@@ -32,32 +32,10 @@
 
 %hr.spacer/
 
-- @instances.each do |instance|
-  .directory__tag
-    = link_to admin_instance_path(instance) do
-      %h4
-        = instance.domain
-        %small
-          - if instance.domain_block
-            - first_item = true
-            - if !instance.domain_block.noop?
-              = t("admin.domain_blocks.severity.#{instance.domain_block.severity}")
-              - first_item = false
-            - unless instance.domain_block.suspend?
-              - if instance.domain_block.reject_media?
-                - unless first_item
-                  •
-                = t('admin.domain_blocks.rejecting_media')
-                - first_item = false
-              - if instance.domain_block.reject_reports?
-                - unless first_item
-                  •
-                = t('admin.domain_blocks.rejecting_reports')
-          - elsif whitelist_mode?
-            = t('admin.accounts.whitelisted')
-          - else
-            = t('admin.accounts.no_limits_imposed')
-      - if instance.countable?
-        .trends__item__current{ title: t('admin.instances.known_accounts', count: instance.accounts_count) }= number_to_human instance.accounts_count, strip_insignificant_zeros: true
+- if @instances.empty?
+  %div.muted-hint.center-text
+    = t 'admin.instances.empty'
+- else
+  = render @instances
 
-= paginate paginated_instances
+= paginate @instances

app/views/admin/instances/show.html.haml

@@ -3,57 +3,59 @@
 
 .dashboard__counters
   %div
-    %div
-      .dashboard__counters__num= number_with_delimiter @following_count
-      .dashboard__counters__label= t 'admin.instances.total_followed_by_them'
-  %div
-    %div
-      .dashboard__counters__num= number_with_delimiter @followers_count
-      .dashboard__counters__label= t 'admin.instances.total_followed_by_us'
-  %div
-    %div
-      .dashboard__counters__num= number_to_human_size @media_storage
-      .dashboard__counters__label= t 'admin.instances.total_storage'
-  %div
-    %div
-      .dashboard__counters__num= number_with_delimiter @blocks_count
-      .dashboard__counters__label= t 'admin.instances.total_blocked_by_us'
+    = link_to admin_accounts_path(remote: '1', by_domain: @instance.domain) do
+      .dashboard__counters__num= number_with_delimiter @instance.accounts_count
+      .dashboard__counters__label= t 'admin.accounts.title'
   %div
     = link_to admin_reports_path(by_target_domain: @instance.domain) do
-      .dashboard__counters__num= number_with_delimiter @reports_count
+      .dashboard__counters__num= number_with_delimiter @instance.reports_count
       .dashboard__counters__label= t 'admin.instances.total_reported'
+  %div
+    %div
+      .dashboard__counters__num= number_to_human_size @instance.media_storage
+      .dashboard__counters__label= t 'admin.instances.total_storage'
+  %div
+    %div
+      .dashboard__counters__num= number_with_delimiter @instance.following_count
+      .dashboard__counters__label= t 'admin.instances.total_followed_by_them'
+  %div
+    %div
+      .dashboard__counters__num= number_with_delimiter @instance.followers_count
+      .dashboard__counters__label= t 'admin.instances.total_followed_by_us'
+  %div
+    %div
+      .dashboard__counters__num= number_with_delimiter @instance.blocks_count
+      .dashboard__counters__label= t 'admin.instances.total_blocked_by_us'
+
   %div
     %div
       .dashboard__counters__num
-        - if @available
+        - if @instance.delivery_failure_tracker.available?
           = fa_icon 'check'
         - else
           = fa_icon 'times'
       .dashboard__counters__label= t 'admin.instances.delivery_available'
 
-- if @private_comment.present?
+- if @instance.private_comment.present?
   .speech-bubble
     .speech-bubble__bubble
-      = simple_format(h(@private_comment))
+      = simple_format(h(@instance.private_comment))
     .speech-bubble__owner= t 'admin.instances.private_comment'
 
-- if @public_comment.present?
+- if @instance.public_comment.present?
   .speech-bubble
     .speech-bubble__bubble
-      = simple_format(h(@public_comment))
+      = simple_format(h(@instance.public_comment))
     .speech-bubble__owner= t 'admin.instances.public_comment'
 
 %hr.spacer/
 
 %div.action-buttons
   %div
-    = link_to t('admin.accounts.title'), admin_accounts_path(remote: '1', by_domain: @instance.domain), class: 'button'
-
-  %div
-    - if @domain_allow
-      = link_to t('admin.domain_allows.undo'), admin_domain_allow_path(@domain_allow), class: 'button button--destructive', data: { confirm: t('admin.accounts.are_you_sure'), method: :delete }
-    - elsif @domain_block
-      = link_to t('admin.domain_blocks.edit'), edit_admin_domain_block_path(@domain_block), class: 'button'
-      = link_to t('admin.domain_blocks.undo'), admin_domain_block_path(@domain_block), class: 'button'
+    - if @instance.domain_allow
+      = link_to t('admin.domain_allows.undo'), admin_domain_allow_path(@instance.domain_allow), class: 'button button--destructive', data: { confirm: t('admin.accounts.are_you_sure'), method: :delete }
+    - elsif @instance.domain_block
+      = link_to t('admin.domain_blocks.edit'), edit_admin_domain_block_path(@instance.domain_block), class: 'button'
+      = link_to t('admin.domain_blocks.undo'), admin_domain_block_path(@instance.domain_block), class: 'button'
     - else
       = link_to t('admin.domain_blocks.add_new'), new_admin_domain_block_path(_domain: @instance.domain), class: 'button'

app/workers/scheduler/instance_refresh_scheduler.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+class Scheduler::InstanceRefreshScheduler
+  include Sidekiq::Worker
+
+  sidekiq_options lock: :until_executed, retry: 0
+
+  def perform
+    Instance.refresh
+  end
+end