From cb69888758cded98a03efbb08f9a78ab2a577abf Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Wed, 27 Jun 2018 11:11:22 +0800 Subject: [PATCH] PERF: Don't pluck all the columns just to retrieve a single value. --- app/serializers/topic_view_serializer.rb | 6 ++---- lib/timeline_lookup.rb | 5 +++-- lib/topic_view.rb | 18 ++++++++++++++---- spec/components/timeline_lookup_spec.rb | 17 ++++++++--------- spec/components/topic_view_spec.rb | 21 +++++++++++++++------ 5 files changed, 42 insertions(+), 25 deletions(-) diff --git a/app/serializers/topic_view_serializer.rb b/app/serializers/topic_view_serializer.rb index 15b23ab66c..a34a22fb50 100644 --- a/app/serializers/topic_view_serializer.rb +++ b/app/serializers/topic_view_serializer.rb @@ -186,10 +186,8 @@ class TopicViewSerializer < ApplicationSerializer end def last_read_post_id - return nil unless object.filtered_post_stream && last_read_post_number - object.filtered_post_stream.each do |ps| - return ps[0] if ps[1] === last_read_post_number - end + return nil unless last_read_post_number + object.last_read_post_id(last_read_post_number) end alias_method :include_last_read_post_id?, :has_topic_user? diff --git a/lib/timeline_lookup.rb b/lib/timeline_lookup.rb index 90d4a5e02a..3a62ba9ef6 100644 --- a/lib/timeline_lookup.rb +++ b/lib/timeline_lookup.rb @@ -1,6 +1,6 @@ module TimelineLookup - # Given an array of tuples (id, post_number, days_ago), return at most `max_values` worth of a + # Given an array of tuples containing (id, days_ago), return at most `max_values` worth of a # lookup table to help the front end timeline display dates associated with posts def self.build(tuples, max_values = 300) result = [] @@ -9,9 +9,10 @@ module TimelineLookup last_days_ago = -1 tuples.each_with_index do |t, idx| + return result unless t.is_a?(Array) next unless (idx % every) === 0 - days_ago = t[2] + days_ago = t[1] if (days_ago != last_days_ago) result << [idx + 1, days_ago] diff --git a/lib/topic_view.rb b/lib/topic_view.rb index 0a0ae5a9aa..8289ccbdf4 100644 --- a/lib/topic_view.rb +++ b/lib/topic_view.rb @@ -374,14 +374,14 @@ class TopicView @filtered_posts.by_newest.with_user.first(25) end - # Returns an array of [id, post_number, days_ago] tuples. + # Returns an array of [id, days_ago] tuples. # `days_ago` is there for the timeline calculations. def filtered_post_stream @filtered_post_stream ||= begin posts = @filtered_posts .order(:sort_order) - columns = [:id, :post_number] + columns = [:id] if !is_mega_topic? columns << 'EXTRACT(DAYS FROM CURRENT_TIMESTAMP - created_at)::INT AS days_ago' @@ -392,7 +392,13 @@ class TopicView end def filtered_post_ids - @filtered_post_ids ||= filtered_post_stream.map { |tuple| tuple[0] } + @filtered_post_ids ||= filtered_post_stream.map do |tuple| + if is_mega_topic? + tuple + else + tuple[0] + end + end end def unfiltered_post_ids @@ -406,6 +412,10 @@ class TopicView end end + def last_read_post_id(post_number) + @filtered_posts.where(post_number: post_number).pluck(:id).first + end + protected def read_posts_set @@ -571,6 +581,6 @@ class TopicView MEGA_TOPIC_POSTS_COUNT = 10000 def is_mega_topic? - @topic.posts_count >= MEGA_TOPIC_POSTS_COUNT + @is_mega_topic ||= (@topic.posts_count >= MEGA_TOPIC_POSTS_COUNT) end end diff --git a/spec/components/timeline_lookup_spec.rb b/spec/components/timeline_lookup_spec.rb index e7df6724ec..c7d69344a7 100644 --- a/spec/components/timeline_lookup_spec.rb +++ b/spec/components/timeline_lookup_spec.rb @@ -6,23 +6,22 @@ describe TimelineLookup do expect(TimelineLookup.build([])).to eq([]) end + it "returns an empty array for if input is an array if post ids" do + expect(TimelineLookup.build([1, 2, 3])).to eq([]) + end + it "returns the lookup for a series of posts" do - result = TimelineLookup.build([[111, 1, 10], [222, 2, 9], [333, 3, 8]]) + result = TimelineLookup.build([[111, 10], [222, 9], [333, 8]]) expect(result).to eq([[1, 10], [2, 9], [3, 8]]) end it "omits duplicate dates" do - result = TimelineLookup.build([[111, 1, 10], [222, 2, 10], [333, 3, 8]]) - expect(result).to eq([[1, 10], [3, 8]]) - end - - it "respects holes in the post numbers" do - result = TimelineLookup.build([[111, 1, 10], [222, 12, 10], [333, 30, 8]]) + result = TimelineLookup.build([[111, 10], [222, 10], [333, 8]]) expect(result).to eq([[1, 10], [3, 8]]) end it "respects a `max_values` setting" do - input = (1..100).map { |i| [1000 + i, i, 100 - i] } + input = (1..100).map { |i| [1000 + i, 100 - i] } result = TimelineLookup.build(input, 5) expect(result.size).to eq(5) @@ -30,7 +29,7 @@ describe TimelineLookup do end it "respects an uneven `max_values` setting" do - input = (1..100).map { |i| [1000 + i, i, 100 - i] } + input = (1..100).map { |i| [1000 + i, 100 - i] } result = TimelineLookup.build(input, 3) expect(result.size).to eq(3) diff --git a/spec/components/topic_view_spec.rb b/spec/components/topic_view_spec.rb index 17e1eedcaf..78f7a32585 100644 --- a/spec/components/topic_view_spec.rb +++ b/spec/components/topic_view_spec.rb @@ -557,9 +557,9 @@ describe TopicView do it 'should return the right columns' do expect(topic_view.filtered_post_stream).to eq([ - [post.id, post.post_number, 0], - [post2.id, post2.post_number, 0], - [post3.id, post3.post_number, 0] + [post.id, 0], + [post2.id, 0], + [post3.id, 0] ]) end @@ -571,9 +571,9 @@ describe TopicView do TopicView.const_set("MEGA_TOPIC_POSTS_COUNT", 2) expect(topic_view.filtered_post_stream).to eq([ - [post.id, post.post_number], - [post2.id, post2.post_number], - [post3.id, post3.post_number] + post.id, + post2.id, + post3.id ]) ensure TopicView.send(:remove_const, "MEGA_TOPIC_POSTS_COUNT") @@ -582,4 +582,13 @@ describe TopicView do end end end + + describe '#last_read_post_id' do + it 'should return the right id' do + post = Fabricate(:post, topic: topic) + + expect(topic_view.last_read_post_id(nil)).to eq(nil) + expect(topic_view.last_read_post_id(post.post_number)).to eq(post.id) + end + end end