PERF: Reduce ActiveRecord allocations in CategoryList#find_relevant_topics (#25950)

Why this change?

Prior to this change, the `CategoryList#find_relevant_topics` method was
loading and allocating all `CategoryFeaturedTopic` records in the
database to eventually only just use its `category_id` and `topic_id`
column. On a site with many `CategoryFeaturedTopic` records, the loading
of the ActiveRecord objects is a source of bottleneck.

The other problem with the `CategoryList#find_relevant_topics` method is
that it is unconditionally loading all records from the database even if
the user does not have access to the category. This again is wasteful.

What does this change do?

This commit makes it such that `CategoryList#find_relevant_topics` is
called only after `CategoryList#find_categories` in the `CategoryList#initialize`
method so that we can filter featured topics against categories that the
user has access to.

The second change is that Instead of loading `CategoryFeaturedTopic` records, we make an
inner join agains the `topics` table instead and skip any allocation of
`CatgoryFeaturedTopic` ActiveRecord objects.

app/models/category_list.rb

@@ -31,8 +31,8 @@ class CategoryList
     @guardian = guardian || Guardian.new
     @options = options
 
-    find_relevant_topics if options[:include_topics]
     find_categories
+    find_relevant_topics if options[:include_topics]
 
     prune_empty
     find_user_data
@@ -71,17 +71,18 @@ class CategoryList
   private
 
   def find_relevant_topics
-    @topics_by_id = {}
-    @topics_by_category_id = {}
-
-    category_featured_topics = CategoryFeaturedTopic.select(%i[category_id topic_id]).order(:rank)
-
     @all_topics =
-      Topic.where(id: category_featured_topics.map(&:topic_id)).includes(
+      Topic
-        :shared_draft,
+        .secured(@guardian)
-        :category,
+        .joins(
-        { topic_thumbnails: %i[optimized_image upload] },
+          "INNER JOIN category_featured_topics ON topics.id = category_featured_topics.topic_id",
         )
+        .where("category_featured_topics.category_id IN (?)", categories_with_descendants.map(&:id))
+        .select(
+          "topics.*, category_featured_topics.category_id AS category_featured_topic_category_id",
+        )
+        .includes(:shared_draft, :category, { topic_thumbnails: %i[optimized_image upload] })
+        .order("category_featured_topics.rank")
 
     @all_topics = @all_topics.joins(:tags).where(tags: { name: @options[:tag] }) if @options[
       :tag
@@ -103,16 +104,18 @@ class CategoryList
     end
 
     @all_topics = TopicQuery.remove_muted_tags(@all_topics, @guardian.user).includes(:last_poster)
+
+    featured_topics_by_category_id = Hash.new { |h, k| h[k] = [] }
+
     @all_topics.each do |t|
       # hint for the serializer
       t.include_last_poster = true
       t.dismissed = dismissed_topic?(t)
-      @topics_by_id[t.id] = t
+      featured_topics_by_category_id[t.category_featured_topic_category_id] << t
     end
 
-    category_featured_topics.each do |cft|
+    categories_with_descendants.each do |category|
-      @topics_by_category_id[cft.category_id] ||= []
+      category.displayable_topics = featured_topics_by_category_id[category.id]
-      @topics_by_category_id[cft.category_id] << cft.topic_id
     end
   end
 
@@ -213,23 +216,6 @@ class CategoryList
       )
       category.has_children = category.subcategories.present?
     end
-
-    if @topics_by_category_id
-      categories_with_descendants.each do |c|
-        topics_in_cat = @topics_by_category_id[c.id]
-        if topics_in_cat.present?
-          c.displayable_topics = []
-          topics_in_cat.each do |topic_id|
-            topic = @topics_by_id[topic_id]
-            if topic.present? && @guardian.can_see?(topic)
-              # topic.category is very slow under rails 4.2
-              topic.association(:category).target = c
-              c.displayable_topics << topic
-            end
-          end
-        end
-      end
-    end
   end
 
   def prune_empty

spec/models/category_list_spec.rb

@@ -27,21 +27,21 @@ RSpec.describe CategoryList do
 
     it "doesn't show topics that you can't view" do
       public_cat = Fabricate(:category_with_definition) # public category
-      Fabricate(:topic, category: public_cat)
+      topic_in_public_cat = Fabricate(:topic, category: public_cat)
 
       private_cat = Fabricate(:category_with_definition) # private category
-      Fabricate(:topic, category: private_cat)
+      topic_in_private_cat = Fabricate(:topic, category: private_cat)
       private_cat.set_permissions(admins: :full)
       private_cat.save
 
       secret_subcat = Fabricate(:category_with_definition, parent_category_id: public_cat.id) # private subcategory
-      Fabricate(:topic, category: secret_subcat)
+      topic_in_secret_subcat = Fabricate(:topic, category: secret_subcat)
       secret_subcat.set_permissions(admins: :full)
       secret_subcat.save
 
       muted_tag = Fabricate(:tag) # muted tag
       SiteSetting.default_tags_muted = muted_tag.name
-      Fabricate(:topic, category: public_cat, tags: [muted_tag])
+      topic_in_public_cat_2 = Fabricate(:topic, category: public_cat, tags: [muted_tag])
 
       muted_tag_2 = Fabricate(:tag)
       TagUser.create!(
@@ -58,16 +58,21 @@ RSpec.describe CategoryList do
           .categories
           .find { |x| x.name == public_cat.name }
           .displayable_topics
-          .count,
+          .map(&:id),
-      ).to eq(3)
+      ).to contain_exactly(
+        topic_in_public_cat.id,
+        topic_in_secret_subcat.id,
+        topic_in_public_cat_2.id,
+      )
+
       expect(
         CategoryList
           .new(Guardian.new(admin), include_topics: true)
           .categories
           .find { |x| x.name == private_cat.name }
           .displayable_topics
-          .count,
+          .map(&:id),
-      ).to eq(1)
+      ).to contain_exactly(topic_in_private_cat.id)
 
       expect(
         CategoryList
@@ -75,8 +80,9 @@ RSpec.describe CategoryList do
           .categories
           .find { |x| x.name == public_cat.name }
           .displayable_topics
-          .count,
+          .map(&:id),
-      ).to eq(2)
+      ).to contain_exactly(topic_in_public_cat.id, topic_in_public_cat_2.id)
+
       expect(
         CategoryList
           .new(Guardian.new(user), include_topics: true)
@@ -90,8 +96,9 @@ RSpec.describe CategoryList do
           .categories
           .find { |x| x.name == public_cat.name }
           .displayable_topics
-          .count,
+          .map(&:id),
-      ).to eq(1)
+      ).to contain_exactly(topic_in_public_cat.id)
+
       expect(
         CategoryList
           .new(Guardian.new(nil), include_topics: true)
@@ -112,8 +119,8 @@ RSpec.describe CategoryList do
           .categories
           .find { |x| x.name == cat.name }
           .displayable_topics
-          .count,
+          .map(&:id),
-      ).to eq(1)
+      ).to contain_exactly(topic.id)
 
       TopicUser.change(user.id, topic.id, notification_level: TopicUser.notification_levels[:muted])