From c9385b9b8eb17710bf0be99ab8ef4740d0a66332 Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Wed, 27 Jul 2016 22:44:03 -0400 Subject: [PATCH 1/2] 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. --- models/comment.rb | 31 +++++++++++++++---------------- models/comment_thread.rb | 27 +++++++++++++-------------- models/constants.rb | 30 ++++++++++++++++++++++++++++++ models/notification.rb | 6 +++++- models/subscription.rb | 11 +++++++---- models/user.rb | 13 ++++++++----- presenters/thread.rb | 2 +- 7 files changed, 79 insertions(+), 41 deletions(-) create mode 100644 models/constants.rb diff --git a/models/comment.rb b/models/comment.rb index 02465d2..76434b1 100644 --- a/models/comment.rb +++ b/models/comment.rb @@ -1,7 +1,7 @@ require_relative 'content' +require_relative 'constants' class Comment < Content - include Mongoid::Tree include Mongoid::Timestamps include Mongoid::MagicCounterCache @@ -25,7 +25,6 @@ class Comment < Content index({author_id: 1, course_id: 1}) index({_type: 1, comment_thread_id: 1, author_id: 1, updated_at: 1}) - index_name Content::ES_INDEX_NAME mapping do @@ -39,7 +38,6 @@ class Comment < Content indexes :updated_at, type: :date, included_in_all: false end - belongs_to :comment_thread, index: true belongs_to :author, class_name: 'User', index: true @@ -91,19 +89,20 @@ class Comment < Content subtree_hash = subtree(sort: sort_by_parent_and_time) self.class.hash_tree(subtree_hash).first else - as_document.slice(*%w[body course_id endorsed endorsement anonymous anonymous_to_peers created_at updated_at at_position_list]) - .merge("id" => _id) - .merge("user_id" => author_id) - .merge("username" => author_username) - .merge("depth" => depth) - .merge("closed" => comment_thread.nil? ? false : comment_thread.closed) - .merge("thread_id" => comment_thread_id) - .merge("parent_id" => parent_ids[-1]) - .merge("commentable_id" => comment_thread.nil? ? nil : comment_thread.commentable_id) - .merge("votes" => votes.slice(*%w[count up_count down_count point])) - .merge("abuse_flaggers" => abuse_flaggers) - .merge("type" => "comment") - .merge("child_count" => get_cached_child_count) + as_document + .slice(BODY, COURSE_ID, ENDORSED, ENDORSEMENT, ANONYMOUS, ANONYMOUS_TO_PEERS, CREATED_AT, UPDATED_AT, AT_POSITION_LIST) + .merge!("id" => _id, + "user_id" => author_id, + "username" => author_username, + "depth" => depth, + "closed" => comment_thread.nil? ? false : comment_thread.closed, + "thread_id" => comment_thread_id, + "parent_id" => parent_ids[-1], + "commentable_id" => comment_thread.nil? ? nil : comment_thread.commentable_id, + "votes" => votes.slice(COUNT, UP_COUNT, DOWN_COUNT, POINT), + "abuse_flaggers" => abuse_flaggers, + "type" => COMMENT, + "child_count" => get_cached_child_count) end end diff --git a/models/comment_thread.rb b/models/comment_thread.rb index d493314..84f2bed 100644 --- a/models/comment_thread.rb +++ b/models/comment_thread.rb @@ -1,9 +1,8 @@ -# -*- coding: utf-8 -*- require 'new_relic/agent/method_tracer' require_relative 'content' +require_relative 'constants' class CommentThread < Content - include Mongoid::Timestamps include Mongoid::Attributes::Dynamic include ActiveModel::MassAssignmentSecurity @@ -32,7 +31,6 @@ class CommentThread < Content index({author_id: 1, course_id: 1}) - index_name Content::ES_INDEX_NAME mapping do @@ -122,17 +120,18 @@ class CommentThread < Content end 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]) - .merge('id' => _id, - 'user_id' => author_id, - 'username' => author_username, - 'votes' => votes.slice(*%w[count up_count down_count point]), - 'abuse_flaggers' => abuse_flaggers, - 'tags' => [], - 'type' => 'thread', - 'group_id' => group_id, - 'pinned' => pinned?, - 'comments_count' => comment_count) + as_document + .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) + .merge!("id" => _id, + "user_id" => author_id, + "username" => author_username, + "votes" => votes.slice(COUNT, UP_COUNT, DOWN_COUNT, POINT), + "abuse_flaggers" => abuse_flaggers, + "tags" => [], + "type" => THREAD, + "group_id" => group_id, + "pinned" => pinned?, + "comments_count" => comment_count) end diff --git a/models/constants.rb b/models/constants.rb new file mode 100644 index 0000000..89533ee --- /dev/null +++ b/models/constants.rb @@ -0,0 +1,30 @@ +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 diff --git a/models/notification.rb b/models/notification.rb index ed5236e..9c91ebe 100644 --- a/models/notification.rb +++ b/models/notification.rb @@ -1,3 +1,5 @@ +require_relative 'constants' + class Notification include Mongoid::Document include Mongoid::Timestamps @@ -14,6 +16,8 @@ class Notification has_and_belongs_to_many :receivers, class_name: "User", inverse_of: :notifications, autosave: true 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 diff --git a/models/subscription.rb b/models/subscription.rb index 8b07575..7cc648e 100644 --- a/models/subscription.rb +++ b/models/subscription.rb @@ -1,18 +1,22 @@ +require_relative 'constants' + class Subscription include Mongoid::Document include Mongoid::Timestamps - + field :subscriber_id, type: String field :source_id, type: String field :source_type, type: String - + index({subscriber_id: 1, source_id: 1, source_type: 1}) index({subscriber_id: 1, source_type: 1}) index({subscriber_id: 1}) index({source_id: 1, source_type: 1}, {background: true}) 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 def subscriber @@ -22,5 +26,4 @@ class Subscription def source source_type.constantize.find(source_id) end - end diff --git a/models/user.rb b/models/user.rb index ff7ab3b..05fd142 100644 --- a/models/user.rb +++ b/models/user.rb @@ -1,4 +1,5 @@ require 'new_relic/agent/method_tracer' +require_relative 'constants' class User include Mongoid::Document @@ -35,18 +36,20 @@ class User end def to_hash(params={}) - hash = as_document.slice(*%w[username external_id]) + hash = as_document + .slice(USERNAME, EXTERNAL_ID) + 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_user_ids" => [], # ditto. "follower_ids" => [], # ditto. "id" => id, "upvoted_ids" => upvoted_ids, "downvoted_ids" => downvoted_ids, - "default_sort_key" => default_sort_key - ) + "default_sort_key" => default_sort_key) end + if params[:course_id] self.class.trace_execution_scoped(['Custom/User.to_hash/count_comments_and_threads']) do if not params[:group_ids].empty? @@ -101,7 +104,7 @@ class User anonymous_to_peers: false ).reject{ |comment| comment.standalone_context? }.count 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 hash diff --git a/presenters/thread.rb b/presenters/thread.rb index 574f404..5eebb6e 100644 --- a/presenters/thread.rb +++ b/presenters/thread.rb @@ -97,7 +97,7 @@ class ThreadPresenter top_level = [] ancestry = [] content.each do |item| - item_hash = item.to_hash.merge("children" => []) + item_hash = item.to_hash.merge!("children" => []) if item.parent_id.nil? top_level << item_hash ancestry = [item_hash] From 1077607dc2d74a895bcfb3f8a0f68ce1d4d59ead Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Fri, 22 Jul 2016 15:38:19 -0400 Subject: [PATCH 2/2] Updated the Mongoid configuration options Updated the Mongoid configuration so that the read and write modes can be specified via the `forum_env`. --- config/mongoid.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/mongoid.yml b/config/mongoid.yml index b5fba53..254a5de 100644 --- a/config/mongoid.yml +++ b/config/mongoid.yml @@ -1,9 +1,9 @@ common: &default_client options: write: - w: 1 + w: <%= ENV['MONGOID_WRITE_MODE'] || 1 %> read: - mode: :primary + mode: :<%= ENV['MONGOID_READ_MODE'] || 'primary' %> max_retries: <%= ENV['MONGOID_MAX_RETRIES'] || 1 %> retry_interval: <%= ENV['MONGOID_RETRY_INTERVAL'] || 0 %> timeout: <%= ENV['MONGOID_TIMEOUT'] || 0.5 %>