From 5d4221fbe1cd2000ac10088c5ea8a8016eb64ad8 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Fri, 8 Sep 2017 13:07:22 +0800 Subject: [PATCH] PERF: Avoid calling expensive `PostGuardian#can_see_post?` multiple times. Before ``` Your Results: (note for timings- percentile is first, duration is second in millisecs) --- topic_admin: 50: 19 75: 19 90: 21 99: 27 topic: 50: 56 75: 62 90: 64 99: 99 timings: load_rails: 1262 ruby-version: 2.4.1-p111 rss_kb: 198432 pss_kb: 136612 virtual: physical architecture: amd64 operatingsystem: Ubuntu memorysize: 15.59 GB kernelversion: 4.10.0 physicalprocessorcount: 1 processor0: Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz rss_kb_9877: 327892 pss_kb_9877: 263671 rss_kb_9946: 325468 pss_kb_9946: 261671 rss_kb_10153: 326456 pss_kb_10153: 262657 ``` After ``` Your Results: (note for timings- percentile is first, duration is second in millisecs) --- topic_admin: 50: 18 75: 18 90: 20 99: 28 topic: 50: 41 75: 42 90: 46 99: 49 timings: load_rails: 1201 ruby-version: 2.4.1-p111 rss_kb: 187936 pss_kb: 123596 virtual: physical architecture: amd64 operatingsystem: Ubuntu memorysize: 15.59 GB kernelversion: 4.10.0 physicalprocessorcount: 1 processor0: Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz rss_kb_26478: 342360 pss_kb_26478: 276696 rss_kb_26547: 340368 pss_kb_26547: 275930 rss_kb_26747: 338964 pss_kb_26747: 274466 ``` --- app/controllers/post_actions_controller.rb | 6 ++-- app/helpers/application_helper.rb | 2 +- app/models/topic.rb | 2 +- app/serializers/basic_user_serializer.rb | 2 +- app/serializers/post_serializer.rb | 32 +++++++++++-------- app/serializers/topic_view_serializer.rb | 6 ++-- lib/guardian/post_guardian.rb | 7 ++-- lib/post_action_creator.rb | 5 ++- spec/components/guardian_spec.rb | 16 +++++++--- .../post_actions_controller_spec.rb | 6 +++- 10 files changed, 52 insertions(+), 32 deletions(-) diff --git a/app/controllers/post_actions_controller.rb b/app/controllers/post_actions_controller.rb index 3cef663c565..7bcad95126a 100644 --- a/app/controllers/post_actions_controller.rb +++ b/app/controllers/post_actions_controller.rb @@ -13,8 +13,10 @@ class PostActionsController < ApplicationController guardian.ensure_post_can_act!( @post, PostActionType.types[@post_action_type_id], - is_warning: params[:is_warning], - taken_actions: taken + opts: { + is_warning: params[:is_warning], + taken_actions: taken + } ) args = {} diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 64316cc4746..6e23d88e788 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -250,7 +250,7 @@ module ApplicationHelper end def crawler_layout? - controller.try(:use_crawler_layout?) + controller&.use_crawler_layout? end def include_crawler_content? diff --git a/app/models/topic.rb b/app/models/topic.rb index 39dca92df6b..b9e1f47ad42 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -247,7 +247,7 @@ class Topic < ActiveRecord::Base def self.visible_post_types(viewed_by = nil) types = Post.types result = [types[:regular], types[:moderator_action], types[:small_action]] - result << types[:whisper] if viewed_by.try(:staff?) + result << types[:whisper] if viewed_by&.staff? result end diff --git a/app/serializers/basic_user_serializer.rb b/app/serializers/basic_user_serializer.rb index 8880c8dbd7e..d14877e6da6 100644 --- a/app/serializers/basic_user_serializer.rb +++ b/app/serializers/basic_user_serializer.rb @@ -9,7 +9,7 @@ class BasicUserSerializer < ApplicationSerializer if Hash === object User.avatar_template(user[:username], user[:uploaded_avatar_id]) else - user.try(:avatar_template) + user&.avatar_template end end diff --git a/app/serializers/post_serializer.rb b/app/serializers/post_serializer.rb index d2e7fc7e863..e03d7891056 100644 --- a/app/serializers/post_serializer.rb +++ b/app/serializers/post_serializer.rb @@ -108,15 +108,15 @@ class PostSerializer < BasicPostSerializer end def moderator? - !!(object.try(:user).try(:moderator?)) + !!(object&.user&.moderator?) end def admin? - !!(object.try(:user).try(:admin?)) + !!(object&.user&.admin?) end def staff? - !!(object.try(:user).try(:staff?)) + !!(object&.user&.staff?) end def yours @@ -140,7 +140,7 @@ class PostSerializer < BasicPostSerializer end def display_username - object.user.try(:name) + object.user&.name end def primary_group_name @@ -154,15 +154,15 @@ class PostSerializer < BasicPostSerializer end def primary_group_flair_url - object.user.try(:primary_group).try(:flair_url) + object.user&.primary_group&.flair_url end def primary_group_flair_bg_color - object.user.try(:primary_group).try(:flair_bg_color) + object.user&.primary_group&.flair_bg_color end def primary_group_flair_color - object.user.try(:primary_group).try(:flair_color) + object.user&.primary_group&.flair_color end def link_counts @@ -189,11 +189,11 @@ class PostSerializer < BasicPostSerializer end def user_title - object.try(:user).try(:title) + object&.user&.title end def trust_level - object.try(:user).try(:trust_level) + object&.user&.trust_level end def reply_to_user @@ -225,14 +225,18 @@ class PostSerializer < BasicPostSerializer # Summary of the actions taken on this post def actions_summary result = [] - PostActionType.types.each do |sym, id| - next if [:bookmark].include?(sym) + can_see_post = scope.can_see_post?(object) + + PostActionType.types.except(:bookmark).each do |sym, id| count_col = "#{sym}_count".to_sym count = object.send(count_col) if object.respond_to?(count_col) summary = { id: id, count: count } summary[:hidden] = true if sym == :vote - summary[:can_act] = true if scope.post_can_act?(object, sym, taken_actions: actions) + + if scope.post_can_act?(object, sym, opts: { taken_actions: actions }, can_see_post: can_see_post) + summary[:can_act] = true + end if sym == :notify_user && scope.current_user.present? && scope.current_user == object.user summary.delete(:can_act) @@ -276,7 +280,7 @@ class PostSerializer < BasicPostSerializer end def include_raw? - @add_raw.present? && (!object.hidden || scope.user.try(:staff?) || yours) + @add_raw.present? && (!object.hidden || scope.user&.staff? || yours) end def include_link_counts? @@ -326,7 +330,7 @@ class PostSerializer < BasicPostSerializer end def is_auto_generated - object.incoming_email.try(:is_auto_generated) + object.incoming_email&.is_auto_generated end def include_is_auto_generated? diff --git a/app/serializers/topic_view_serializer.rb b/app/serializers/topic_view_serializer.rb index 228a19c8352..c033c0ad233 100644 --- a/app/serializers/topic_view_serializer.rb +++ b/app/serializers/topic_view_serializer.rb @@ -83,7 +83,7 @@ class TopicViewSerializer < ApplicationSerializer result[:allowed_users] = object.topic.allowed_users.select do |user| !allowed_user_ids.include?(user.id) - end.map do |user| + end.map! do |user| BasicUserSerializer.new(user, scope: scope, root: false) end end @@ -94,7 +94,7 @@ class TopicViewSerializer < ApplicationSerializer end end - if object.suggested_topics.try(:topics).present? + if object.suggested_topics&.topics.present? result[:suggested_topics] = object.suggested_topics.topics.map do |t| SuggestedTopicSerializer.new(t, scope: scope, root: false) end @@ -215,7 +215,7 @@ class TopicViewSerializer < ApplicationSerializer def actions_summary result = [] - return [] unless post = object.posts.try(:first) + return [] unless post = object.posts&.first PostActionType.topic_flag_types.each do |sym, id| result << { id: id, count: 0, diff --git a/lib/guardian/post_guardian.rb b/lib/guardian/post_guardian.rb index da4275650c5..cd274e0d056 100644 --- a/lib/guardian/post_guardian.rb +++ b/lib/guardian/post_guardian.rb @@ -3,9 +3,8 @@ module PostGuardian # Can the user act on the post in a particular way. # taken_actions = the list of actions the user has already taken - def post_can_act?(post, action_key, opts = {}) - - return false unless can_see_post?(post) + def post_can_act?(post, action_key, opts: {}, can_see_post: nil) + return false unless (can_see_post.nil? && can_see_post?(post)) || can_see_post # no warnings except for staff return false if (action_key == :notify_user && !is_staff? && opts[:is_warning].present? && opts[:is_warning] == 'true') @@ -187,7 +186,7 @@ module PostGuardian end def can_vote?(post, opts = {}) - post_can_act?(post, :vote, opts) + post_can_act?(post, :vote, opts: opts) end def can_change_post_owner? diff --git a/lib/post_action_creator.rb b/lib/post_action_creator.rb index e995db1a298..32e4b2aa13a 100644 --- a/lib/post_action_creator.rb +++ b/lib/post_action_creator.rb @@ -7,7 +7,10 @@ class PostActionCreator end def perform(action) - guardian.ensure_post_can_act!(@post, action, taken_actions: PostAction.counts_for([@post].compact, @user)[@post.try(:id)]) + guardian.ensure_post_can_act!(@post, action, opts: { + taken_actions: PostAction.counts_for([@post].compact, @user)[@post&.id] + }) + PostAction.act(@user, @post, action) end diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb index 87a3413c4f7..03f6d9a75b5 100644 --- a/spec/components/guardian_spec.rb +++ b/spec/components/guardian_spec.rb @@ -57,11 +57,15 @@ describe Guardian do end it "returns false when you've already done it" do - expect(Guardian.new(user).post_can_act?(post, :like, taken_actions: { PostActionType.types[:like] => 1 })).to be_falsey + expect(Guardian.new(user).post_can_act?(post, :like, opts: { + taken_actions: { PostActionType.types[:like] => 1 } + })).to be_falsey end it "returns false when you already flagged a post" do - expect(Guardian.new(user).post_can_act?(post, :off_topic, taken_actions: { PostActionType.types[:spam] => 1 })).to be_falsey + expect(Guardian.new(user).post_can_act?(post, :off_topic, opts: { + taken_actions: { PostActionType.types[:spam] => 1 } + })).to be_falsey end it "returns false for notify_user if private messages are disabled" do @@ -863,11 +867,15 @@ describe Guardian do end it "doesn't allow voting if the user has an action from voting already" do - expect(guardian.post_can_act?(post, :vote, taken_actions: { PostActionType.types[:vote] => 1 })).to be_falsey + expect(guardian.post_can_act?(post, :vote, opts: { + taken_actions: { PostActionType.types[:vote] => 1 } + })).to be_falsey end it "allows voting if the user has performed a different action" do - expect(guardian.post_can_act?(post, :vote, taken_actions: { PostActionType.types[:like] => 1 })).to be_truthy + expect(guardian.post_can_act?(post, :vote, opts: { + taken_actions: { PostActionType.types[:like] => 1 } + })).to be_truthy end it "isn't allowed on archived topics" do diff --git a/spec/controllers/post_actions_controller_spec.rb b/spec/controllers/post_actions_controller_spec.rb index 01259e72f91..a4b335f2042 100644 --- a/spec/controllers/post_actions_controller_spec.rb +++ b/spec/controllers/post_actions_controller_spec.rb @@ -53,7 +53,11 @@ describe PostActionsController do it "passes a list of taken actions through" do PostAction.create(post_id: @post.id, user_id: @user.id, post_action_type_id: PostActionType.types[:inappropriate]) - Guardian.any_instance.expects(:post_can_act?).with(@post, :off_topic, has_entry(taken_actions: has_key(PostActionType.types[:inappropriate]))) + + Guardian.any_instance.expects(:post_can_act?).with(@post, :off_topic, + has_entry(opts: has_entry(taken_actions: has_key(PostActionType.types[:inappropriate]))) + ) + xhr :post, :create, id: @post.id, post_action_type_id: PostActionType.types[:off_topic] end