Commit c9385b9b by Toby Lawrence

Reduced merges, do them in place, and reduce allocations.

Primarily, we wanted to reduce the one-thing-per-merge calls, but also,
we want to reduce allocations overall, both from merge calls which will
duplicate the source hash and from string constants sprinkled
throughout.

Seemingly, defining a constant-style word list is not the ticket.  For
whatever reason, Ruby can't seem to figure out the list doesn't change,
and presumably is splating out all of the words and allocating them as
if they were declared right there.  Wahhhh.

Now we're using a straight up one-for-one constant definition of each
string, in a big constants file, where we freeze the strings on the
spot.  We then reference each constant directly at the callsite, which
was the original style and, ultimately, is the best for readability.
parent 6a443e79
require_relative 'content' require_relative 'content'
require_relative 'constants'
class Comment < Content class Comment < Content
include Mongoid::Tree include Mongoid::Tree
include Mongoid::Timestamps include Mongoid::Timestamps
include Mongoid::MagicCounterCache include Mongoid::MagicCounterCache
...@@ -25,7 +25,6 @@ class Comment < Content ...@@ -25,7 +25,6 @@ class Comment < Content
index({author_id: 1, course_id: 1}) index({author_id: 1, course_id: 1})
index({_type: 1, comment_thread_id: 1, author_id: 1, updated_at: 1}) index({_type: 1, comment_thread_id: 1, author_id: 1, updated_at: 1})
index_name Content::ES_INDEX_NAME index_name Content::ES_INDEX_NAME
mapping do mapping do
...@@ -39,7 +38,6 @@ class Comment < Content ...@@ -39,7 +38,6 @@ class Comment < Content
indexes :updated_at, type: :date, included_in_all: false indexes :updated_at, type: :date, included_in_all: false
end end
belongs_to :comment_thread, index: true belongs_to :comment_thread, index: true
belongs_to :author, class_name: 'User', index: true belongs_to :author, class_name: 'User', index: true
...@@ -91,19 +89,20 @@ class Comment < Content ...@@ -91,19 +89,20 @@ class Comment < Content
subtree_hash = subtree(sort: sort_by_parent_and_time) subtree_hash = subtree(sort: sort_by_parent_and_time)
self.class.hash_tree(subtree_hash).first self.class.hash_tree(subtree_hash).first
else else
as_document.slice(*%w[body course_id endorsed endorsement anonymous anonymous_to_peers created_at updated_at at_position_list]) as_document
.merge("id" => _id) .slice(BODY, COURSE_ID, ENDORSED, ENDORSEMENT, ANONYMOUS, ANONYMOUS_TO_PEERS, CREATED_AT, UPDATED_AT, AT_POSITION_LIST)
.merge("user_id" => author_id) .merge!("id" => _id,
.merge("username" => author_username) "user_id" => author_id,
.merge("depth" => depth) "username" => author_username,
.merge("closed" => comment_thread.nil? ? false : comment_thread.closed) "depth" => depth,
.merge("thread_id" => comment_thread_id) "closed" => comment_thread.nil? ? false : comment_thread.closed,
.merge("parent_id" => parent_ids[-1]) "thread_id" => comment_thread_id,
.merge("commentable_id" => comment_thread.nil? ? nil : comment_thread.commentable_id) "parent_id" => parent_ids[-1],
.merge("votes" => votes.slice(*%w[count up_count down_count point])) "commentable_id" => comment_thread.nil? ? nil : comment_thread.commentable_id,
.merge("abuse_flaggers" => abuse_flaggers) "votes" => votes.slice(COUNT, UP_COUNT, DOWN_COUNT, POINT),
.merge("type" => "comment") "abuse_flaggers" => abuse_flaggers,
.merge("child_count" => get_cached_child_count) "type" => COMMENT,
"child_count" => get_cached_child_count)
end end
end end
......
# -*- coding: utf-8 -*-
require 'new_relic/agent/method_tracer' require 'new_relic/agent/method_tracer'
require_relative 'content' require_relative 'content'
require_relative 'constants'
class CommentThread < Content class CommentThread < Content
include Mongoid::Timestamps include Mongoid::Timestamps
include Mongoid::Attributes::Dynamic include Mongoid::Attributes::Dynamic
include ActiveModel::MassAssignmentSecurity include ActiveModel::MassAssignmentSecurity
...@@ -32,7 +31,6 @@ class CommentThread < Content ...@@ -32,7 +31,6 @@ class CommentThread < Content
index({author_id: 1, course_id: 1}) index({author_id: 1, course_id: 1})
index_name Content::ES_INDEX_NAME index_name Content::ES_INDEX_NAME
mapping do mapping do
...@@ -122,17 +120,18 @@ class CommentThread < Content ...@@ -122,17 +120,18 @@ class CommentThread < Content
end end
def to_hash(params={}) def to_hash(params={})
as_document.slice(*%w[thread_type title body course_id anonymous anonymous_to_peers commentable_id created_at updated_at at_position_list closed context last_activity_at]) as_document
.merge('id' => _id, .slice(THREAD_TYPE, TITLE, BODY, COURSE_ID, ANONYMOUS, ANONYMOUS_TO_PEERS, COMMENTABLE_ID, CREATED_AT, UPDATED_AT, AT_POSITION_LIST, CLOSED, CONTEXT, LAST_ACTIVITY_AT)
'user_id' => author_id, .merge!("id" => _id,
'username' => author_username, "user_id" => author_id,
'votes' => votes.slice(*%w[count up_count down_count point]), "username" => author_username,
'abuse_flaggers' => abuse_flaggers, "votes" => votes.slice(COUNT, UP_COUNT, DOWN_COUNT, POINT),
'tags' => [], "abuse_flaggers" => abuse_flaggers,
'type' => 'thread', "tags" => [],
'group_id' => group_id, "type" => THREAD,
'pinned' => pinned?, "group_id" => group_id,
'comments_count' => comment_count) "pinned" => pinned?,
"comments_count" => comment_count)
end end
......
BODY = "body".freeze
COURSE_ID = "course_id".freeze
ENDORSED = "endorsed".freeze
ENDORSEMENT = "endorsement".freeze
ANONYMOUS = "anonymous".freeze
ANONYMOUS_TO_PEERS = "anonymous_to_peers".freeze
CREATED_AT = "created_at".freeze
UPDATED_AT = "updated_at".freeze
AT_POSITION_LIST = "at_position_list".freeze
THREAD_TYPE = "thread_type".freeze
TITLE = "title".freeze
COMMENTABLE_ID = "commentable_id".freeze
CLOSED = "closed".freeze
CONTEXT = "context".freeze
LAST_ACTIVITY_AT = "last_activity_at".freeze
NOTIFICATION_TYPE = "notification_type".freeze
INFO = "info".freeze
ACTOR_ID = "actor_id".freeze
TARGET_ID = "target_id".freeze
SUBSCRIBER_ID = "subscriber_id".freeze
SOURCE_ID = "source_id".freeze
SOURCE_TYPE = "source_type".freeze
COUNT = "count".freeze
UP_COUNT = "up_count".freeze
DOWN_COUNT = "down_count".freeze
POINT = "point".freeze
USERNAME = "username".freeze
EXTERNAL_ID = "external_id".freeze
COMMENT = "comment".freeze
THREAD = "thread".freeze
require_relative 'constants'
class Notification class Notification
include Mongoid::Document include Mongoid::Document
include Mongoid::Timestamps include Mongoid::Timestamps
...@@ -14,6 +16,8 @@ class Notification ...@@ -14,6 +16,8 @@ class Notification
has_and_belongs_to_many :receivers, class_name: "User", inverse_of: :notifications, autosave: true has_and_belongs_to_many :receivers, class_name: "User", inverse_of: :notifications, autosave: true
def to_hash(params={}) def to_hash(params={})
as_document.slice(*%w[notification_type info actor_id target_id]).merge("id" => _id) as_document
.slice(NOTIFICATION_TYPE, INFO, ACTOR_ID, TARGET_ID)
.merge!("id" => _id)
end end
end end
require_relative 'constants'
class Subscription class Subscription
include Mongoid::Document include Mongoid::Document
include Mongoid::Timestamps include Mongoid::Timestamps
field :subscriber_id, type: String field :subscriber_id, type: String
field :source_id, type: String field :source_id, type: String
field :source_type, type: String field :source_type, type: String
index({subscriber_id: 1, source_id: 1, source_type: 1}) index({subscriber_id: 1, source_id: 1, source_type: 1})
index({subscriber_id: 1, source_type: 1}) index({subscriber_id: 1, source_type: 1})
index({subscriber_id: 1}) index({subscriber_id: 1})
index({source_id: 1, source_type: 1}, {background: true}) index({source_id: 1, source_type: 1}, {background: true})
def to_hash def to_hash
as_document.slice(*%w[subscriber_id source_id source_type]).merge("id" => _id) as_document
.slice(SUBSCRIBER_ID, SOURCE_ID, SOURCE_TYPE)
.merge!("id" => _id)
end end
def subscriber def subscriber
...@@ -22,5 +26,4 @@ class Subscription ...@@ -22,5 +26,4 @@ class Subscription
def source def source
source_type.constantize.find(source_id) source_type.constantize.find(source_id)
end end
end end
require 'new_relic/agent/method_tracer' require 'new_relic/agent/method_tracer'
require_relative 'constants'
class User class User
include Mongoid::Document include Mongoid::Document
...@@ -35,18 +36,20 @@ class User ...@@ -35,18 +36,20 @@ class User
end end
def to_hash(params={}) def to_hash(params={})
hash = as_document.slice(*%w[username external_id]) hash = as_document
.slice(USERNAME, EXTERNAL_ID)
if params[:complete] if params[:complete]
hash = hash.merge("subscribed_thread_ids" => subscribed_thread_ids, hash = hash.merge!("subscribed_thread_ids" => subscribed_thread_ids,
"subscribed_commentable_ids" => [], # not used by comment client. To be removed once removed from comment client. "subscribed_commentable_ids" => [], # not used by comment client. To be removed once removed from comment client.
"subscribed_user_ids" => [], # ditto. "subscribed_user_ids" => [], # ditto.
"follower_ids" => [], # ditto. "follower_ids" => [], # ditto.
"id" => id, "id" => id,
"upvoted_ids" => upvoted_ids, "upvoted_ids" => upvoted_ids,
"downvoted_ids" => downvoted_ids, "downvoted_ids" => downvoted_ids,
"default_sort_key" => default_sort_key "default_sort_key" => default_sort_key)
)
end end
if params[:course_id] if params[:course_id]
self.class.trace_execution_scoped(['Custom/User.to_hash/count_comments_and_threads']) do self.class.trace_execution_scoped(['Custom/User.to_hash/count_comments_and_threads']) do
if not params[:group_ids].empty? if not params[:group_ids].empty?
...@@ -101,7 +104,7 @@ class User ...@@ -101,7 +104,7 @@ class User
anonymous_to_peers: false anonymous_to_peers: false
).reject{ |comment| comment.standalone_context? }.count ).reject{ |comment| comment.standalone_context? }.count
end end
hash = hash.merge("threads_count" => threads_count, "comments_count" => comments_count) hash = hash.merge!("threads_count" => threads_count, "comments_count" => comments_count)
end end
end end
hash hash
......
...@@ -97,7 +97,7 @@ class ThreadPresenter ...@@ -97,7 +97,7 @@ class ThreadPresenter
top_level = [] top_level = []
ancestry = [] ancestry = []
content.each do |item| content.each do |item|
item_hash = item.to_hash.merge("children" => []) item_hash = item.to_hash.merge!("children" => [])
if item.parent_id.nil? if item.parent_id.nil?
top_level << item_hash top_level << item_hash
ancestry = [item_hash] ancestry = [item_hash]
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment