From d13c6c46763f7627b4167388e3e53d4893d96da7 Mon Sep 17 00:00:00 2001 From: Peter Bhat Harkins Date: Fri, 6 Oct 2023 23:32:31 -0500 Subject: [PATCH] comments: fewer db round trips on creation Writes a vote directly to avoid vote_thusly doing round trips to check if one exists, etc. Removes redundant transactions from controllers from #899. Rails already creates a transaction for the .save. Unifies Story cache updating. Previously recalculate_hotness! was called twice on comment creation. Moves comment counting into the db. Shorter transaction should reduce the frequence of lobsters/lobsters-ansible/issues/39 but seems unlikely to eliminate it as the create + upvote transactions for stories + comments still read/write from stories, comments, and votes. --- app/controllers/comments_controller.rb | 2 +- app/controllers/stories_controller.rb | 2 +- app/models/comment.rb | 20 ++++++++------------ app/models/story.rb | 24 +++++++----------------- spec/models/story_spec.rb | 4 ++-- 5 files changed, 19 insertions(+), 33 deletions(-) diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb index 2921ff40e9..3b19e7319d 100644 --- a/app/controllers/comments_controller.rb +++ b/app/controllers/comments_controller.rb @@ -51,7 +51,7 @@ def create content_type: "text/html", locals: {comment: comment} end - if comment.valid? && params[:preview].blank? && ActiveRecord::Base.transaction { comment.save } + if comment.valid? && params[:preview].blank? && comment.save comment.current_vote = {vote: 1} render_created_comment(comment) else diff --git a/app/controllers/stories_controller.rb b/app/controllers/stories_controller.rb index 3a0605e835..5b3f7c9601 100644 --- a/app/controllers/stories_controller.rb +++ b/app/controllers/stories_controller.rb @@ -20,7 +20,7 @@ def create @story.attributes = story_params if @story.valid? && !(@story.already_posted_recently? && !@story.seen_previous) - if ActiveRecord::Base.transaction { @story.save } + if @story.save ReadRibbon.where(user: @user, story: @story).first_or_create return redirect_to @story.comments_path end diff --git a/app/models/comment.rb b/app/models/comment.rb index 76f9679ca3..df321d6d49 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -29,10 +29,6 @@ class Comment < ApplicationRecord end after_create :record_initial_upvote, :mark_submitter, :deliver_reply_notifications, :deliver_mention_notifications, :log_hat_use - after_create do - # fire this once after record_initial_upvote - update_score_and_recalculate! 0, 0 - end after_destroy :unassign_votes scope :deleted, -> { where(is_deleted: true) } @@ -259,7 +255,7 @@ def delete_for_user(user, reason = nil) save(validate: false) Comment.record_timestamps = true - story.update_comments_count! + story.update_cached_columns self.user.refresh_counts! end @@ -371,7 +367,7 @@ def update_score_and_recalculate!(score_delta, flag_delta) confidence_order = concat(lpad(char(65536 - floor(((confidence - -0.2) * 65535) / 1.2) using binary), 2, '0'), char(id & 0xff using binary)) WHERE id = #{id.to_i} SQL - story.recalculate_hotness! + story.update_cached_columns end def gone_text @@ -504,11 +500,11 @@ def plaintext_comment end def record_initial_upvote - Vote.vote_thusly_on_story_or_comment_for_user_because( - 1, story_id, id, user_id, nil, false - ) + # not calling vote_thusly to save round-trips of validations + Vote.create! story: story, comment: self, user: user, vote: 1 + update_score_and_recalculate! 0, 0 # trigger db calculation - story.update_comments_count! + story.update_cached_columns end def score_for_user(u) @@ -541,7 +537,7 @@ def to_param end def unassign_votes - story.update_comments_count! + story.update_cached_columns end def url @@ -592,7 +588,7 @@ def undelete_for_user(user) save(validate: false) Comment.record_timestamps = true - story.update_comments_count! + story.update_cached_columns self.user.refresh_counts! end diff --git a/app/models/story.rb b/app/models/story.rb index 9ed1fb053c..caef2bdbab 100644 --- a/app/models/story.rb +++ b/app/models/story.rb @@ -157,7 +157,7 @@ class Story < ApplicationRecord before_save :log_moderation before_save :fix_bogus_chars after_create :mark_submitter, :record_initial_upvote - after_save :update_merged_into_story_comments, :recalculate_hotness!, :update_story_text + after_save :update_cached_columns, :update_story_text validate do if url.present? @@ -270,8 +270,8 @@ def most_recent_similar def self.recalculate_all_hotnesses! # do the front page first, since find_each can't take an order - Story.order("id DESC").limit(100).each(&:recalculate_hotness!) - Story.find_each(&:recalculate_hotness!) + Story.order("id DESC").limit(100).each(&:update_cached_columns) + Story.find_each(&:update_cached_columns) true end @@ -620,10 +620,6 @@ def merge_story_short_id merged_story_id ? merged_into_story.try(:short_id) : nil end - def recalculate_hotness! - update_column :hotness, calculated_hotness - end - def record_initial_upvote Vote.vote_thusly_on_story_or_comment_for_user_because(1, id, nil, user_id, nil, false) end @@ -812,17 +808,11 @@ def update_availability end end - def update_comments_count! - comments = merged_comments + def update_cached_columns + update_column :comments_count, merged_comments.active.count + merged_into_story&.update_cached_columns - # calculate count after removing deleted comments and threads - update_column :comments_count, (comments.count { |c| !c.is_gone? }) - update_merged_into_story_comments - recalculate_hotness! - end - - def update_merged_into_story_comments - merged_into_story&.update_comments_count! + update_column :hotness, calculated_hotness end def update_story_text diff --git a/spec/models/story_spec.rb b/spec/models/story_spec.rb index 0be0aeced1..74064346b6 100644 --- a/spec/models/story_spec.rb +++ b/spec/models/story_spec.rb @@ -405,7 +405,7 @@ def fake_response(content, type, code = "200") end end - describe "#update_comments_count!" do + describe "#update_cached_columns" do context "with a merged_into_story" do let(:merged_into_story) { create(:story) } let(:story) { create(:story, merged_into_story: merged_into_story) } @@ -414,7 +414,7 @@ def fake_response(content, type, code = "200") expect(story.comments_count).to eq 0 expect(merged_into_story.comments_count).to eq 0 create(:comment, story: story) - story.update_comments_count! + story.update_cached_columns expect(story.comments_count).to eq 1 expect(merged_into_story.comments_count).to eq 1 end