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
```
This commit is contained in:
Guo Xiang Tan 2017-09-08 13:07:22 +08:00
parent 164a388dcc
commit 5d4221fbe1
10 changed files with 52 additions and 32 deletions

View File

@ -13,8 +13,10 @@ class PostActionsController < ApplicationController
guardian.ensure_post_can_act!( guardian.ensure_post_can_act!(
@post, @post,
PostActionType.types[@post_action_type_id], PostActionType.types[@post_action_type_id],
is_warning: params[:is_warning], opts: {
taken_actions: taken is_warning: params[:is_warning],
taken_actions: taken
}
) )
args = {} args = {}

View File

@ -250,7 +250,7 @@ module ApplicationHelper
end end
def crawler_layout? def crawler_layout?
controller.try(:use_crawler_layout?) controller&.use_crawler_layout?
end end
def include_crawler_content? def include_crawler_content?

View File

@ -247,7 +247,7 @@ class Topic < ActiveRecord::Base
def self.visible_post_types(viewed_by = nil) def self.visible_post_types(viewed_by = nil)
types = Post.types types = Post.types
result = [types[:regular], types[:moderator_action], types[:small_action]] 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 result
end end

View File

@ -9,7 +9,7 @@ class BasicUserSerializer < ApplicationSerializer
if Hash === object if Hash === object
User.avatar_template(user[:username], user[:uploaded_avatar_id]) User.avatar_template(user[:username], user[:uploaded_avatar_id])
else else
user.try(:avatar_template) user&.avatar_template
end end
end end

View File

@ -108,15 +108,15 @@ class PostSerializer < BasicPostSerializer
end end
def moderator? def moderator?
!!(object.try(:user).try(:moderator?)) !!(object&.user&.moderator?)
end end
def admin? def admin?
!!(object.try(:user).try(:admin?)) !!(object&.user&.admin?)
end end
def staff? def staff?
!!(object.try(:user).try(:staff?)) !!(object&.user&.staff?)
end end
def yours def yours
@ -140,7 +140,7 @@ class PostSerializer < BasicPostSerializer
end end
def display_username def display_username
object.user.try(:name) object.user&.name
end end
def primary_group_name def primary_group_name
@ -154,15 +154,15 @@ class PostSerializer < BasicPostSerializer
end end
def primary_group_flair_url def primary_group_flair_url
object.user.try(:primary_group).try(:flair_url) object.user&.primary_group&.flair_url
end end
def primary_group_flair_bg_color def primary_group_flair_bg_color
object.user.try(:primary_group).try(:flair_bg_color) object.user&.primary_group&.flair_bg_color
end end
def primary_group_flair_color def primary_group_flair_color
object.user.try(:primary_group).try(:flair_color) object.user&.primary_group&.flair_color
end end
def link_counts def link_counts
@ -189,11 +189,11 @@ class PostSerializer < BasicPostSerializer
end end
def user_title def user_title
object.try(:user).try(:title) object&.user&.title
end end
def trust_level def trust_level
object.try(:user).try(:trust_level) object&.user&.trust_level
end end
def reply_to_user def reply_to_user
@ -225,14 +225,18 @@ class PostSerializer < BasicPostSerializer
# Summary of the actions taken on this post # Summary of the actions taken on this post
def actions_summary def actions_summary
result = [] result = []
PostActionType.types.each do |sym, id| can_see_post = scope.can_see_post?(object)
next if [:bookmark].include?(sym)
PostActionType.types.except(:bookmark).each do |sym, id|
count_col = "#{sym}_count".to_sym count_col = "#{sym}_count".to_sym
count = object.send(count_col) if object.respond_to?(count_col) count = object.send(count_col) if object.respond_to?(count_col)
summary = { id: id, count: count } summary = { id: id, count: count }
summary[:hidden] = true if sym == :vote 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 if sym == :notify_user && scope.current_user.present? && scope.current_user == object.user
summary.delete(:can_act) summary.delete(:can_act)
@ -276,7 +280,7 @@ class PostSerializer < BasicPostSerializer
end end
def include_raw? def include_raw?
@add_raw.present? && (!object.hidden || scope.user.try(:staff?) || yours) @add_raw.present? && (!object.hidden || scope.user&.staff? || yours)
end end
def include_link_counts? def include_link_counts?
@ -326,7 +330,7 @@ class PostSerializer < BasicPostSerializer
end end
def is_auto_generated def is_auto_generated
object.incoming_email.try(:is_auto_generated) object.incoming_email&.is_auto_generated
end end
def include_is_auto_generated? def include_is_auto_generated?

View File

@ -83,7 +83,7 @@ class TopicViewSerializer < ApplicationSerializer
result[:allowed_users] = object.topic.allowed_users.select do |user| result[:allowed_users] = object.topic.allowed_users.select do |user|
!allowed_user_ids.include?(user.id) !allowed_user_ids.include?(user.id)
end.map do |user| end.map! do |user|
BasicUserSerializer.new(user, scope: scope, root: false) BasicUserSerializer.new(user, scope: scope, root: false)
end end
end end
@ -94,7 +94,7 @@ class TopicViewSerializer < ApplicationSerializer
end end
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| result[:suggested_topics] = object.suggested_topics.topics.map do |t|
SuggestedTopicSerializer.new(t, scope: scope, root: false) SuggestedTopicSerializer.new(t, scope: scope, root: false)
end end
@ -215,7 +215,7 @@ class TopicViewSerializer < ApplicationSerializer
def actions_summary def actions_summary
result = [] result = []
return [] unless post = object.posts.try(:first) return [] unless post = object.posts&.first
PostActionType.topic_flag_types.each do |sym, id| PostActionType.topic_flag_types.each do |sym, id|
result << { id: id, result << { id: id,
count: 0, count: 0,

View File

@ -3,9 +3,8 @@ module PostGuardian
# Can the user act on the post in a particular way. # Can the user act on the post in a particular way.
# taken_actions = the list of actions the user has already taken # taken_actions = the list of actions the user has already taken
def post_can_act?(post, action_key, opts = {}) 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
return false unless can_see_post?(post)
# no warnings except for staff # no warnings except for staff
return false if (action_key == :notify_user && !is_staff? && opts[:is_warning].present? && opts[:is_warning] == 'true') return false if (action_key == :notify_user && !is_staff? && opts[:is_warning].present? && opts[:is_warning] == 'true')
@ -187,7 +186,7 @@ module PostGuardian
end end
def can_vote?(post, opts = {}) def can_vote?(post, opts = {})
post_can_act?(post, :vote, opts) post_can_act?(post, :vote, opts: opts)
end end
def can_change_post_owner? def can_change_post_owner?

View File

@ -7,7 +7,10 @@ class PostActionCreator
end end
def perform(action) 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) PostAction.act(@user, @post, action)
end end

View File

@ -57,11 +57,15 @@ describe Guardian do
end end
it "returns false when you've already done it" do 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 end
it "returns false when you already flagged a post" do 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 end
it "returns false for notify_user if private messages are disabled" do it "returns false for notify_user if private messages are disabled" do
@ -863,11 +867,15 @@ describe Guardian do
end end
it "doesn't allow voting if the user has an action from voting already" do 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 end
it "allows voting if the user has performed a different action" do 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 end
it "isn't allowed on archived topics" do it "isn't allowed on archived topics" do

View File

@ -53,7 +53,11 @@ describe PostActionsController do
it "passes a list of taken actions through" 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]) 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] xhr :post, :create, id: @post.id, post_action_type_id: PostActionType.types[:off_topic]
end end