From b7023da8942e23e237e385b233536f4d3155daa4 Mon Sep 17 00:00:00 2001
From: Sam <sam.saffron@gmail.com>
Date: Wed, 17 Jan 2018 15:49:35 +1100
Subject: [PATCH] PERF: reduce queries required for post timings

- also freezes a bunch of strings
- bypass active record for an exists query
---
 app/models/post_timing.rb     | 18 +++++++++++----
 app/models/user_stat.rb       | 41 +++++++++++++++++++----------------
 spec/models/user_stat_spec.rb | 10 ++++++---
 3 files changed, 43 insertions(+), 26 deletions(-)

diff --git a/app/models/post_timing.rb b/app/models/post_timing.rb
index b32c80ccfcc..54ed9bbefcf 100644
--- a/app/models/post_timing.rb
+++ b/app/models/post_timing.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+#
 require_dependency 'archetype'
 
 class PostTiming < ActiveRecord::Base
@@ -76,7 +78,7 @@ class PostTiming < ActiveRecord::Base
   MAX_READ_TIME_PER_BATCH = 60 * 1000.0
 
   def self.process_timings(current_user, topic_id, topic_time, timings, opts = {})
-    current_user.user_stat.update_time_read!
+    UserStat.update_time_read!(current_user.id)
 
     max_time_per_post = ((Time.now - current_user.created_at) * 1000.0)
     max_time_per_post = MAX_READ_TIME_PER_BATCH if max_time_per_post > MAX_READ_TIME_PER_BATCH
@@ -103,8 +105,7 @@ class PostTiming < ActiveRecord::Base
     end
 
     if join_table.length > 0
-      sql = <<SQL
-
+      sql = <<~SQL
       UPDATE post_timings t
       SET msecs = t.msecs + x.msecs
       FROM (#{join_table.join(" UNION ALL ")}) x
@@ -117,7 +118,16 @@ SQL
       result = exec_sql(sql)
       result.type_map = SqlBuilder.pg_type_map
       existing = Set.new(result.column_values(0))
-      new_posts_read = timings.size - existing.size if Topic.where(id: topic_id, archetype: Archetype.default).exists?
+
+      sql = <<~SQL
+      SELECT 1 FROM topics
+      WHERE deleted_at IS NULL AND
+        archetype = 'regular' AND
+        id = :topic_id
+      SQL
+
+      is_regular = Post.exec_sql(sql, topic_id: topic_id).cmd_tuples == 1
+      new_posts_read = timings.size - existing.size if is_regular
 
       timings.each_with_index do |(post_number, time), index|
         unless existing.include?(index)
diff --git a/app/models/user_stat.rb b/app/models/user_stat.rb
index 8e7b3da43b5..cf9ab05e2ca 100644
--- a/app/models/user_stat.rb
+++ b/app/models/user_stat.rb
@@ -1,3 +1,4 @@
+# frozen_string_literal: true
 class UserStat < ActiveRecord::Base
 
   belongs_to :user
@@ -68,42 +69,44 @@ class UserStat < ActiveRecord::Base
 
   MAX_TIME_READ_DIFF = 100
   # attempt to add total read time to user based on previous time this was called
-  def update_time_read!
-    if last_seen = last_seen_cached
+  def self.update_time_read!(id)
+    if last_seen = last_seen_cached(id)
       diff = (Time.now.to_f - last_seen.to_f).round
       if diff > 0 && diff < MAX_TIME_READ_DIFF
         update_args = ["time_read = time_read + ?", diff]
-        UserStat.where(user_id: id, time_read: time_read).update_all(update_args)
+        UserStat.where(user_id: id).update_all(update_args)
         UserVisit.where(user_id: id, visited_at: Time.zone.now.to_date).update_all(update_args)
       end
     end
-    cache_last_seen(Time.now.to_f)
+    cache_last_seen(id, Time.now.to_f)
+  end
+
+  def update_time_read!
+    UserStat.update_time_read!(id)
   end
 
   def reset_bounce_score!
     update_columns(reset_bounce_score_after: nil, bounce_score: 0)
   end
 
+  def self.last_seen_key(id)
+    # frozen
+    -"user-last-seen:#{id}"
+  end
+
+  def self.last_seen_cached(id)
+    $redis.get(last_seen_key(id))
+  end
+
+  def self.cache_last_seen(id, val)
+    $redis.set(last_seen_key(id), val)
+  end
+
   protected
 
   def trigger_badges
     BadgeGranter.queue_badge_grant(Badge::Trigger::UserChange, user: self.user)
   end
-
-  private
-
-  def last_seen_key
-    @last_seen_key ||= "user-last-seen:#{id}"
-  end
-
-  def last_seen_cached
-    $redis.get(last_seen_key)
-  end
-
-  def cache_last_seen(val)
-    $redis.set(last_seen_key, val)
-  end
-
 end
 
 # == Schema Information
diff --git a/spec/models/user_stat_spec.rb b/spec/models/user_stat_spec.rb
index ccb9638d7a7..43e73819a14 100644
--- a/spec/models/user_stat_spec.rb
+++ b/spec/models/user_stat_spec.rb
@@ -77,22 +77,26 @@ describe UserStat do
     let(:stat) { user.user_stat }
 
     it 'makes no changes if nothing is cached' do
-      stat.expects(:last_seen_cached).returns(nil)
+      $redis.del(UserStat.last_seen_key(user.id))
       stat.update_time_read!
       stat.reload
       expect(stat.time_read).to eq(0)
     end
 
     it 'makes a change if time read is below threshold' do
-      stat.expects(:last_seen_cached).returns(Time.now - 10)
+      freeze_time
+      UserStat.cache_last_seen(user.id, (Time.now - 10).to_f)
       stat.update_time_read!
       stat.reload
       expect(stat.time_read).to eq(10)
     end
 
     it 'makes no change if time read is above threshold' do
+      freeze_time
+
       t = Time.now - 1 - UserStat::MAX_TIME_READ_DIFF
-      stat.expects(:last_seen_cached).returns(t)
+      UserStat.cache_last_seen(user.id, t.to_f)
+
       stat.update_time_read!
       stat.reload
       expect(stat.time_read).to eq(0)