Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PERF: Dematerialize topic_reply_count #9769

Merged
merged 5 commits into from May 14, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions app/assets/javascripts/admin/models/tl3-requirements.js
Expand Up @@ -12,6 +12,11 @@ export default EmberObject.extend({
return Math.round((minDaysVisited * 100) / timePeriod);
},

@discourseComputed("num_topics_replied_to", "min_topics_replied_to")
capped_topics_replied_to(numReplied, minReplied) {
return numReplied > minReplied;
},

@discourseComputed(
"days_visited",
"min_days_visited",
Expand Down
Expand Up @@ -33,7 +33,7 @@
<tr>
<th>{{i18n "admin.user.tl3_requirements.topics_replied_to"}}</th>
<td>{{check-icon model.tl3Requirements.met.topics_replied_to}}</td>
<td>{{model.tl3Requirements.num_topics_replied_to}}</td>
<td>{{#if model.tl3Requirements.capped_topics_replied_to}}&ge; {{/if}}{{model.tl3Requirements.num_topics_replied_to}}</td>
<td>{{model.tl3Requirements.min_topics_replied_to}}</td>
</tr>
<tr>
Expand Down
Expand Up @@ -20,21 +20,19 @@ export default DesktopNotificationConfig.extend({
"isNotSupported",
"isEnabled",
"bannerDismissed",
"currentUser.reply_count",
"currentUser.topic_count"
"currentUser.any_posts"
)
showNotificationPromptBanner(
isNotSupported,
isEnabled,
bannerDismissed,
replyCount,
topicCount
anyPosts
) {
return (
this.siteSettings.push_notifications_prompt &&
!isNotSupported &&
this.currentUser &&
replyCount + topicCount > 0 &&
anyPosts &&
Notification.permission !== "denied" &&
Notification.permission !== "granted" &&
!isEnabled &&
Expand Down
7 changes: 1 addition & 6 deletions app/assets/javascripts/discourse/app/controllers/composer.js
Expand Up @@ -731,12 +731,7 @@ export default Controller.extend({

this.close();

const currentUser = this.currentUser;
if (composer.creatingTopic) {
currentUser.set("topic_count", currentUser.topic_count + 1);
} else {
currentUser.set("reply_count", currentUser.reply_count + 1);
}
this.currentUser.set("any_posts", true);

const post = result.target;
if (post && !staged) {
Expand Down
1 change: 0 additions & 1 deletion app/jobs/scheduled/tl3_promotions.rb
Expand Up @@ -34,7 +34,6 @@ def execute(args)
).where.not(id: demoted_user_ids)
.joins(:user_stat)
.where("user_stats.days_visited >= ?", SiteSetting.tl3_requires_days_visited)
.where("user_stats.topic_reply_count >= ?", SiteSetting.tl3_requires_topics_replied_to)
.where("user_stats.topics_entered >= ?", SiteSetting.tl3_requires_topics_viewed_all_time)
.where("user_stats.posts_read_count >= ?", SiteSetting.tl3_requires_posts_read_all_time)
.where("user_stats.likes_given >= ?", SiteSetting.tl3_requires_likes_given)
Expand Down
6 changes: 1 addition & 5 deletions app/models/trust_level3_requirements.rb
Expand Up @@ -143,11 +143,7 @@ def min_days_visited
end

def num_topics_replied_to
@user.posts
.public_posts
.where("posts.created_at > ? AND posts.post_number > 1", time_period.days.ago)
.select("distinct topic_id")
.count
@user.user_stat.calc_topic_reply_count!(min_topics_replied_to + 1, time_period.days.ago)
end

def min_topics_replied_to
Expand Down
34 changes: 27 additions & 7 deletions app/models/user_stat.rb
Expand Up @@ -4,6 +4,9 @@ class UserStat < ActiveRecord::Base
belongs_to :user
after_save :trigger_badges

# TODO(2021-05-13): Remove
self.ignored_columns = ["topic_reply_count"]

def self.ensure_consistency!(last_seen = 1.hour.ago)
reset_bounce_scores
update_distinct_badge_count
Expand Down Expand Up @@ -151,12 +154,30 @@ def update_distinct_badge_count
end

# topic_reply_count is a count of posts in other users' topics
def update_topic_reply_count
self.topic_reply_count = Topic
.joins("INNER JOIN posts ON topics.id = posts.topic_id AND topics.user_id <> posts.user_id")
.where("posts.deleted_at IS NULL AND posts.user_id = ?", self.user_id)
.distinct
.count
def calc_topic_reply_count!(max, start_time = nil)
sql = <<~SQL
SELECT COUNT(*) count
FROM (
SELECT DISTINCT posts.topic_id
FROM posts
INNER JOIN topics ON topics.id = posts.topic_id
WHERE posts.user_id = ?
AND topics.user_id <> posts.user_id
AND posts.deleted_at IS NULL AND topics.deleted_at IS NULL
AND topics.archetype <> 'private_message'
ZogStriP marked this conversation as resolved.
Show resolved Hide resolved
#{start_time.nil? ? '' : 'AND posts.created_at > ?'}
LIMIT ?
) as user_topic_replies
SQL
if start_time.nil?
DB.query_single(sql, self.user_id, max).first
else
DB.query_single(sql, self.user_id, start_time, max).first
end
end

def any_posts
user.posts.exists?
end

MAX_TIME_READ_DIFF = 100
Expand Down Expand Up @@ -212,7 +233,6 @@ def trigger_badges
# posts_read_count :integer default(0), not null
# likes_given :integer default(0), not null
# likes_received :integer default(0), not null
# topic_reply_count :integer default(0), not null
# new_since :datetime not null
# read_faq :datetime
# first_post_created_at :datetime
Expand Down
11 changes: 3 additions & 8 deletions app/serializers/current_user_serializer.rb
Expand Up @@ -12,8 +12,7 @@ class CurrentUserSerializer < BasicUserSerializer
:moderator?,
:staff?,
:title,
:reply_count,
:topic_count,
:any_posts,
:enable_quoting,
:enable_defer,
:external_links_in_new_tab,
Expand Down Expand Up @@ -66,12 +65,8 @@ def read_faq
object.user_stat.read_faq?
end

def topic_count
object.user_stat.topic_count
end

def reply_count
object.user_stat.topic_reply_count
def any_posts
object.user_stat.any_posts
end

def hide_profile_and_presence
Expand Down
19 changes: 19 additions & 0 deletions db/post_migrate/20200513185052_drop_topic_reply_count.rb
@@ -0,0 +1,19 @@
# frozen_string_literal: true

class DropTopicReplyCount < ActiveRecord::Migration[6.0]
DROPPED_COLUMNS ||= {
user_stats: %i{
topic_reply_count
}
}

def up
DROPPED_COLUMNS.each do |table, columns|
Migration::ColumnDropper.execute_drop(table, columns)
end
end

def down
raise ActiveRecord::IrreversibleMigration
end
end
5 changes: 0 additions & 5 deletions lib/post_creator.rb
Expand Up @@ -539,11 +539,6 @@ def update_user_counts
@user.user_stat.topic_count += 1 if @post.is_first_post?
end

# We don't count replies to your own topics
if !@opts[:import_mode] && @user.id != @topic.user_id
@user.user_stat.update_topic_reply_count
end

@user.user_stat.save!

if !@topic.private_message? && @post.post_type != Post.types[:whisper]
Expand Down
5 changes: 0 additions & 5 deletions lib/post_destroyer.rb
Expand Up @@ -350,11 +350,6 @@ def update_user_counts
author.user_stat.topic_count -= 1 if @post.is_first_post?
end

# We don't count replies to your own topics in topic_reply_count
if @topic && author.id != @topic.user_id
author.user_stat.update_topic_reply_count
end

author.user_stat.save!

if @post.created_at == author.last_posted_at
Expand Down
2 changes: 0 additions & 2 deletions lib/post_revisor.rb
Expand Up @@ -394,7 +394,6 @@ def update_post
prev_owner_user_stat.topic_count -= 1 if @post.is_first_post?
prev_owner_user_stat.likes_received -= likes
end
prev_owner_user_stat.update_topic_reply_count

if @post.created_at == prev_owner.user_stat.first_post_created_at
prev_owner_user_stat.first_post_created_at = prev_owner.posts.order('created_at ASC').first.try(:created_at)
Expand All @@ -408,7 +407,6 @@ def update_post
new_owner_user_stat.topic_count += 1 if @post.is_first_post?
new_owner_user_stat.likes_received += likes
end
new_owner_user_stat.update_topic_reply_count
new_owner_user_stat.save!
end
end
Expand Down
2 changes: 1 addition & 1 deletion lib/promotion.rb
Expand Up @@ -91,7 +91,7 @@ def self.tl2_met?(user)
return false if stat.days_visited < SiteSetting.tl2_requires_days_visited
return false if stat.likes_received < SiteSetting.tl2_requires_likes_received
return false if stat.likes_given < SiteSetting.tl2_requires_likes_given
return false if stat.topic_reply_count < SiteSetting.tl2_requires_topic_reply_count
return false if stat.calc_topic_reply_count!(SiteSetting.tl2_requires_topic_reply_count) < SiteSetting.tl2_requires_topic_reply_count

true
end
Expand Down
18 changes: 1 addition & 17 deletions lib/tasks/import.rake
Expand Up @@ -227,28 +227,13 @@ def update_user_stats

# TODO: topic_count is counting all topics you replied in as if you started the topic.
# TODO: post_count is counting first posts.
# TODO: topic_reply_count is never used, and is counting PMs here.
DB.exec <<-SQL
WITH X AS (
SELECT p.user_id
, COUNT(p.id) posts
, COUNT(DISTINCT p.topic_id) topics
, MIN(p.created_at) min_created_at
, COALESCE(COUNT(DISTINCT DATE(p.created_at)), 0) days
, COALESCE((
SELECT COUNT(*)
FROM topics
WHERE id IN (
SELECT topic_id
FROM posts p2
JOIN topics t2 ON t2.id = p2.topic_id
WHERE p2.deleted_at IS NULL
AND p2.post_type = 1
AND NOT COALESCE(p2.hidden, 't')
AND p2.user_id <> t2.user_id
AND p2.user_id = p.user_id
)
), 0) topic_replies
FROM posts p
JOIN topics t ON t.id = p.topic_id
WHERE p.deleted_at IS NULL
Expand All @@ -268,7 +253,6 @@ def update_user_stats
, topics_entered = X.topics
, first_post_created_at = X.min_created_at
, days_visited = X.days
, topic_reply_count = X.topic_replies
FROM X
WHERE user_stats.user_id = X.user_id
AND (post_count <> X.posts
Expand All @@ -278,7 +262,7 @@ def update_user_stats
OR topics_entered <> X.topics
OR COALESCE(first_post_created_at, '1970-01-01') <> X.min_created_at
OR days_visited <> X.days
OR topic_reply_count <> X.topic_replies)
)
SQL
end

Expand Down
27 changes: 0 additions & 27 deletions script/import_scripts/base.rb
Expand Up @@ -58,7 +58,6 @@ def perform
update_post_timings
update_feature_topic_users
update_category_featured_topics
update_topic_count_replies
reset_topic_counters
end

Expand Down Expand Up @@ -690,19 +689,6 @@ def update_last_posted_at
end

def update_user_stats
puts "", "Updating topic reply counts..."

count = 0
total = User.real.count

User.real.find_each do |u|
u.create_user_stat if u.user_stat.nil?
us = u.user_stat
us.update_topic_reply_count
us.save
print_status(count += 1, total, get_start_time("user_stats"))
end

puts "", "Updating first_post_created_at..."

DB.exec <<~SQL
Expand Down Expand Up @@ -807,19 +793,6 @@ def update_category_featured_topics
end
end

def update_topic_count_replies
puts "", "Updating user topic reply counts"

count = 0
total = User.real.count

User.real.find_each do |u|
u.user_stat.update_topic_reply_count
u.user_stat.save!
print_status(count += 1, total, get_start_time("topic_count_replies"))
end
end

def update_tl0
puts "", "Setting users with no posts to trust level 0"

Expand Down
22 changes: 0 additions & 22 deletions spec/components/post_creator_spec.rb
Expand Up @@ -253,28 +253,6 @@
creator_with_image_sizes.create
end

it 'increases topic response counts' do
first_post = creator.create

# ensure topic user is correct
topic_user = first_post.user.topic_users.find_by(topic_id: first_post.topic_id)
expect(topic_user).to be_present
expect(topic_user).to be_posted
expect(topic_user.last_read_post_number).to eq(first_post.post_number)
expect(topic_user.highest_seen_post_number).to eq(first_post.post_number)

user2 = Fabricate(:coding_horror)
expect(user2.user_stat.topic_reply_count).to eq(0)

expect(first_post.user.user_stat.reload.topic_reply_count).to eq(0)

PostCreator.new(user2, topic_id: first_post.topic_id, raw: "this is my test post 123").create

expect(first_post.user.user_stat.reload.topic_reply_count).to eq(0)

expect(user2.user_stat.reload.topic_reply_count).to eq(1)
end

it 'sets topic excerpt if first post, but not second post' do
first_post = creator.create
topic = first_post.topic.reload
Expand Down
13 changes: 11 additions & 2 deletions spec/components/promotion_spec.rb
Expand Up @@ -124,14 +124,19 @@
context "that has done the requisite things" do

before do
SiteSetting.tl2_requires_topic_reply_count = 3

stat = user.user_stat
stat.topics_entered = SiteSetting.tl2_requires_topics_entered
stat.posts_read_count = SiteSetting.tl2_requires_read_posts
stat.time_read = SiteSetting.tl2_requires_time_spent_mins * 60
stat.days_visited = SiteSetting.tl2_requires_days_visited * 60
stat.likes_received = SiteSetting.tl2_requires_likes_received
stat.likes_given = SiteSetting.tl2_requires_likes_given
stat.topic_reply_count = SiteSetting.tl2_requires_topic_reply_count
SiteSetting.tl2_requires_topic_reply_count.times do |_|
topic = Fabricate(:topic)
reply = Fabricate(:post, topic: topic, user: user, post_number: 2)
end

@result = promotion.review
end
Expand All @@ -148,6 +153,7 @@
context "when the account hasn't existed long enough" do
it "does not promote the user" do
user.created_at = 1.minute.ago
SiteSetting.tl2_requires_topic_reply_count = 3

stat = user.user_stat
stat.topics_entered = SiteSetting.tl2_requires_topics_entered
Expand All @@ -156,7 +162,10 @@
stat.days_visited = SiteSetting.tl2_requires_days_visited * 60
stat.likes_received = SiteSetting.tl2_requires_likes_received
stat.likes_given = SiteSetting.tl2_requires_likes_given
stat.topic_reply_count = SiteSetting.tl2_requires_topic_reply_count
SiteSetting.tl2_requires_topic_reply_count.times do |_|
topic = Fabricate(:topic)
reply = Fabricate(:post, topic: topic, user: user, post_number: 2)
end

result = promotion.review
expect(result).to eq(false)
Expand Down