PERF: calculate topic_counts for tags in an async job so tag queries that include counts are much faster

app/controllers/tags_controller.rb

@@ -25,51 +25,34 @@ class TagsController < ::ApplicationController
     respond_to do |format|
 
       format.html do
-        tag_counts = self.class.tags_by_count(guardian, limit: 300).count(Tag::COUNT_ARG)
-        @tags = self.class.tag_counts_json(tag_counts)
         render :index
       end
 
       format.json do
         if SiteSetting.tags_listed_by_group
-          # TODO: performance is bad
-          grouped_tag_counts = TagGroup.order('name ASC').preload(:tags).map do |tag_group|
-            h = Tag.tags_by_count_query(limit: 300)
-              .joins("LEFT JOIN tag_group_memberships ON tags.id = tag_group_memberships.tag_id")
-              .where('tag_group_memberships.tag_group_id = ?', tag_group.id)
-              .count(Tag::COUNT_ARG)
-            { id: tag_group.id, name: tag_group.name, tags: self.class.tag_counts_json(h) }
+          grouped_tag_counts = TagGroup.order('name ASC').includes(:tags).map do |tag_group|
+            { id: tag_group.id, name: tag_group.name, tags: self.class.tag_counts_json(tag_group.tags) }
           end
 
-          ungrouped_tag_counts = guardian.filter_allowed_categories(
-            Tag.tags_by_count_query(limit: 300)
-              .where("tags.id NOT IN (select tag_id from tag_group_memberships)")
-              .count(Tag::COUNT_ARG)
-          )
+          ungrouped_tags = Tag.where("tags.id NOT IN (select tag_id from tag_group_memberships)")
 
           render json: {
-            tags: self.class.tag_counts_json(ungrouped_tag_counts), # tags that don't belong to a group
+            tags: self.class.tag_counts_json(ungrouped_tags), # tags that don't belong to a group
             extras: { tag_groups: grouped_tag_counts }
           }
         else
-          unrestricted_tag_counts = guardian.filter_allowed_categories(
-            Tag.tags_by_count_query(limit: 300)
-              .where("tags.id NOT IN (select tag_id from category_tags)")
-              .count(Tag::COUNT_ARG)
-          )
+          unrestricted_tags = Tag.where("tags.id NOT IN (select tag_id from category_tags)")
 
           categories = Category.where("id in (select category_id from category_tags)")
             .where("id in (?)", guardian.allowed_category_ids)
-            .preload(:tags)
+            .includes(:tags)
 
           category_tag_counts = categories.map do |c|
-            h = Tag.category_tags_by_count_query(c, limit: 300).count(Tag::COUNT_ARG)
-            h.merge!(c.tags.where.not(name: h.keys).inject({}) { |sum, t| sum[t.name] = 0; sum }) # unused tags
-            { id: c.id, tags: self.class.tag_counts_json(h) }
+            { id: c.id, tags: self.class.tag_counts_json(c.tags) }
           end
 
           render json: {
-            tags: self.class.tag_counts_json(unrestricted_tag_counts),
+            tags: self.class.tag_counts_json(unrestricted_tags),
             extras: { categories: category_tag_counts }
           }
         end
@@ -160,7 +143,7 @@ class TagsController < ::ApplicationController
     category = params[:categoryId] ? Category.find_by_id(params[:categoryId]) : nil
 
     tags_with_counts = DiscourseTagging.filter_allowed_tags(
-      Tag.tags_by_count_query(params.slice(:limit)),
+      Tag.order('topic_count DESC').limit(params[:limit]),
       guardian,
       for_input: params[:filterForInput],
       term: params[:q],
@@ -168,7 +151,7 @@ class TagsController < ::ApplicationController
       selected_tags: params[:selected_tags]
     )
 
-    tags = tags_with_counts.count(Tag::COUNT_ARG).map { |t, c| { id: t, text: t, count: c } }
+    tags = self.class.tag_counts_json(tags_with_counts)
 
     json_response = { results: tags }
 
@@ -211,12 +194,8 @@ class TagsController < ::ApplicationController
       raise Discourse::NotFound unless SiteSetting.tagging_enabled?
     end
 
-    def self.tags_by_count(guardian, opts = {})
-      guardian.filter_allowed_categories(Tag.tags_by_count_query(opts))
-    end
-
-    def self.tag_counts_json(tag_counts)
-      tag_counts.map { |t, c| { id: t, text: t, count: c } }
+    def self.tag_counts_json(tags)
+      tags.map { |t| { id: t.name, text: t.name, count: t.topic_count } }
     end
 
     def set_category_from_params

app/jobs/scheduled/ensure_db_consistency.rb

@@ -15,6 +15,7 @@ module Jobs
       Badge.ensure_consistency!
       CategoryUser.ensure_consistency!
       UserOption.ensure_consistency!
+      Tag.ensure_consistency!
     end
   end
 end

app/models/tag.rb

@@ -31,9 +31,24 @@ class Tag < ActiveRecord::Base
     q
   end
 
-  def self.category_tags_by_count_query(category, opts = {})
-    tags_by_count_query(opts).where("tags.id in (select tag_id from category_tags where category_id = ?)", category.id)
-      .where("topics.category_id = ?", category.id)
+  def self.ensure_consistency!
+    update_topic_counts # topic_count counter cache can miscount
+  end
+
+  def self.update_topic_counts
+    Category.exec_sql <<~SQL
+      UPDATE tags t
+      SET topic_count = x.topic_count
+      FROM (
+        SELECT COUNT(topics.id) AS topic_count, tags.id AS tag_id
+        FROM tags
+        LEFT JOIN topic_tags ON tags.id = topic_tags.tag_id
+        LEFT JOIN topics ON topics.id = topic_tags.topic_id AND topics.deleted_at IS NULL
+        GROUP BY tags.id
+      ) x
+      WHERE x.tag_id = t.id
+        AND x.topic_count <> t.topic_count
+    SQL
   end
 
   def self.top_tags(limit_arg: nil, category: nil, guardian: nil)

app/views/tags/index.html.erb

@@ -1,24 +1,3 @@
-<% if crawler_layout? %>
-
-<% content_for :title do %><%=t "tags.title" %> - <%= SiteSetting.title %><% end %>
-
-<h1><%=t "tags.title" %></h1>
-
-<div class='tags-list' itemscope itemtype='http://schema.org/ItemList'>
-  <% @tags.each do |tag| %>
-    <div itemprop='itemListElement' itemscope itemtype='http://schema.org/ListItem'>
-      <meta itemprop='url' content='<%= "#{Discourse.base_url}/tags/#{tag[:id]}" %>'>
-      <a href='<%= "#{Discourse.base_url}/tags/#{tag[:id]}" %>' itemprop='item'>
-        <span itemprop='name'>
-          <%= tag[:text] %>
-        </span>
-      </a>
-    </div>
-  <% end %>
-</div>
-
-<% end %>
-
 <% content_for :head do %>
   <%= raw crawlable_meta_data(title: @title, description: @description_meta) %>
 <% end %>

test/javascripts/acceptance/tags-test.js.es6

@@ -8,4 +8,57 @@ QUnit.test("list the tags", assert => {
     assert.ok($('body.tags-page').length, "has the body class");
     assert.ok(exists('.tag-eviltrout'), "shows the evil trout tag");
   });
-});
+});
+
+acceptance("Tags listed by group", {
+  loggedIn: true,
+  settings: {
+    tags_listed_by_group: true
+  }
+});
+
+
+QUnit.test("list the tags in groups", assert => {
+  server.get('/tags', () => { // eslint-disable-line no-undef
+    return [
+      200,
+      { "Content-Type": "application/json" },
+      {
+        "tags":[{id: 'planned', text: 'planned', count: 7}],
+        "extras": { "tag_groups": [
+          {id: 2, name: "Ford Cars", tags: [
+            {id: 'escort', text: 'escort', count: 1},
+            {id: 'focus', text: 'focus', count: 3}
+          ]},
+          {id: 1, name: "Honda Cars", tags: [
+            {id: 'civic', text: 'civic', count: 4},
+            {id: 'accord', text: 'accord', count: 2}
+          ]},
+          {id: 1, name: "Makes", tags: [
+            {id: 'ford', text: 'ford', count: 5},
+            {id: 'honda', text: 'honda', count: 6}
+          ]}
+        ]}
+      }
+    ];
+  });
+
+  visit('/tags');
+  andThen(() => {
+    assert.equal($('.tag-list').length, 4, "shows separate lists for the 3 groups and the ungrouped tags");
+    assert.ok(
+      _.isEqual(
+        _.map($('.tag-list h3'), i => { return $(i).text(); } ),
+        ['Ford Cars', 'Honda Cars', 'Makes', 'Other Tags']
+      ),
+      'shown in given order and with tags that are not in a group'
+    );
+    assert.ok(
+      _.isEqual(
+        _.map($('.tag-list:first .discourse-tag'), i => { return $(i).text(); }),
+        ['focus', 'escort']
+      ),
+      'shows the tags in default sort (by count)'
+    );
+  });
+});