From 8370b26cbacef93b3c51219252e0b3f21db7c849 Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 23 Feb 2015 16:50:52 +1100 Subject: [PATCH] PERF: optimise pinned handling on home page Old query used to scan the full topics table, on home page Instead we now perform 2 queries, one for pinned and one for unpinned and merge results in a 10x improvement on a 1 million topic DB --- app/models/category_featured_topic.rb | 2 +- app/models/topic_list.rb | 16 +----- lib/topic_query.rb | 75 +++++++++++++++++---------- 3 files changed, 49 insertions(+), 44 deletions(-) diff --git a/app/models/category_featured_topic.rb b/app/models/category_featured_topic.rb index 485fcfd9e..66c05573f 100644 --- a/app/models/category_featured_topic.rb +++ b/app/models/category_featured_topic.rb @@ -25,7 +25,7 @@ class CategoryFeaturedTopic < ActiveRecord::Base visible: true, no_definitions: true) - results = query.list_category(c).topic_ids.uniq + results = query.list_category_topic_ids(c).uniq return if results == existing diff --git a/app/models/topic_list.rb b/app/models/topic_list.rb index 1e6a8bd57..0fb296171 100644 --- a/app/models/topic_list.rb +++ b/app/models/topic_list.rb @@ -32,16 +32,7 @@ class TopicList def topics return @topics if @topics.present? - # copy side-loaded data (allowed users) before dumping it with the .to_a - @topics_input.each do |t| - t.allowed_user_ids = if @filter == :private_messages - t.allowed_users.map { |u| u.id }.to_a - else - [] - end - end - - @topics = @topics_input.to_a + @topics = @topics_input # Attach some data for serialization to each topic @topic_lookup = TopicUser.lookup_for(@current_user, @topics) if @current_user @@ -88,11 +79,6 @@ class TopicList @topics end - def topic_ids - return [] unless @topics_input - @topics_input.pluck(:id) - end - def attributes {'more_topics_url' => page} end diff --git a/lib/topic_query.rb b/lib/topic_query.rb index 3485e59d8..0e38b881c 100644 --- a/lib/topic_query.rb +++ b/lib/topic_query.rb @@ -62,7 +62,7 @@ class TopicQuery end builder.add_results(random_suggested(topic, builder.results_left, builder.excluded_topic_ids)) unless builder.full? - create_list(:suggested, {}, builder.results) + create_list(:suggested, {unordered: true}, builder.results) end # The latest view of topics @@ -77,11 +77,11 @@ class TopicQuery end def list_new - create_list(:new, {}, new_results) + create_list(:new, {unordered: true}, new_results) end def list_unread - create_list(:unread, {}, unread_results) + create_list(:unread, {unordered: true}, unread_results) end def list_posted @@ -128,13 +128,11 @@ class TopicQuery end def list_category(category) - create_list(:category, unordered: true, category: category.id) do |list| - if @user - list.order(TopicQuerySQL.order_with_pinned_sql) - else - list.order(TopicQuerySQL.order_basic_bumped) - end - end + create_list(:category, unordered: true, category: category.id) + end + + def list_category_topic_ids(category) + default_results(unordered: true, category: category.id).pluck(:id) end def list_new_in_category(category) @@ -154,10 +152,43 @@ class TopicQuery .where("COALESCE(tu.notification_level, :regular) >= :tracking", regular: TopicUser.notification_levels[:regular], tracking: TopicUser.notification_levels[:tracking]) end + def prioritize_pinned_topics(topics, options) + + pinned_clause = options[:category] ? "" : "pinned_globally AND " + pinned_clause << " pinned_at IS NOT NULL " + if @user + pinned_clause << " AND (topics.pinned_at > tu.cleared_pinned_at OR tu.cleared_pinned_at IS NULL)" + end + + unpinned_topics = topics.where("NOT ( #{pinned_clause} )") + pinned_topics = topics.where(pinned_clause) + + per_page = options[:per_page] || per_page_setting + limit = per_page unless options[:limit] == false + page = options[:page].to_i + + if page == 0 + (pinned_topics + unpinned_topics)[0...limit] if limit + else + unpinned_topics.offset((page * per_page - pinned_topics.count) - 1).to_a + end + + end + def create_list(filter, options={}, topics = nil) topics ||= default_results(options) topics = yield(topics) if block_given? - list = TopicList.new(filter, @user, topics, options.merge(@options)) + + options = options.merge(@options) + if (options[:order] || "activity") == "activity" && !options[:unordered] + topics = prioritize_pinned_topics(topics, options) + end + + topics = topics.to_a.each do |t| + t.allowed_user_ids = filter == :private_messags ? t.allowed_users.map{|u| u.id} : [] + end + + list = TopicList.new(filter, @user, topics.to_a, options.merge(@options)) list.per_page = per_page_setting list end @@ -171,11 +202,12 @@ class TopicQuery def unread_results(options={}) result = TopicQuery.unread_filter(default_results(options.reverse_merge(:unordered => true))) .order('CASE WHEN topics.user_id = tu.user_id THEN 1 ELSE 2 END') - suggested_ordering(result, options) end def new_results(options={}) + # TODO does this make sense or should it be ordered on created_at + # it is ordering on bumped_at now result = TopicQuery.new_filter(default_results(options.reverse_merge(:unordered => true)), @user.treat_as_new_topic_start_date) result = remove_muted_categories(result, @user, exclude: options[:category]) suggested_ordering(result, options) @@ -196,7 +228,7 @@ class TopicQuery result = Topic.includes(:allowed_users) .where("topics.id IN (SELECT topic_id FROM topic_allowed_users WHERE user_id = #{user.id.to_i})") .joins("LEFT OUTER JOIN topic_users AS tu ON (topics.id = tu.topic_id AND tu.user_id = #{user.id.to_i})") - .order(TopicQuerySQL.order_nocategory_basic_bumped) + .order("topics.bumped_at") .private_messages result = result.limit(options[:per_page]) unless options[:limit] == false @@ -205,18 +237,6 @@ class TopicQuery result end - def default_ordering(result, options) - # If we're logged in, we have to pay attention to our pinned settings - if @user - result = options[:category].blank? ? result.order(TopicQuerySQL.order_nocategory_with_pinned_sql) : - result.order(TopicQuerySQL.order_with_pinned_sql) - else - result = options[:category].blank? ? result.order(TopicQuerySQL.order_nocategory_basic_bumped) : - result.order(TopicQuerySQL.order_basic_bumped) - end - result - end - def apply_ordering(result, options) sort_column = SORTABLE_MAPPING[options[:order]] || 'default' sort_dir = (options[:ascending] == "true") ? "ASC" : "DESC" @@ -228,14 +248,13 @@ class TopicQuery # If something requires a custom order, for example "unread" which sorts the least read # to the top, do nothing return result if options[:unordered] - # Otherwise apply our default ordering - return default_ordering(result, options) end sort_column = 'bumped_at' end # If we are sorting by category, actually use the name if sort_column == 'category_id' + # TODO forces a table scan, slow return result.references(:categories).order(TopicQuerySQL.order_by_category_sql(sort_dir)) end @@ -404,6 +423,6 @@ class TopicQuery result = result.order("CASE WHEN topics.category_id = #{options[:topic].category_id.to_i} THEN 0 ELSE 1 END") end - result.order(TopicQuerySQL.order_nocategory_with_pinned_sql) + result.order('topics.bumped_at DESC') end end