broadcast policy perf improvements, fixes SD-2130

jenseng · jenseng · commit f005ba96a062 · 2017-03-01T20:42:36.000Z
don't create extra AR objects or incur unnecessary write_attribute taxes (super expensive for serialized columns). this speeds up saving any AR model that has a broadcast policy (whether or not messages are sent), in particular ones having serialized columns (e.g. quiz submissions), since we avoid an unnecessary type cast on each attribute. while it's a little hard to quantify the perf boost we'll see in the app, here is a sampling of numbers we see in the specs: spec/messages : 0:48 -> 0:45 (6.6% reduction) spec/models/a* : 5:59 -> 5:17 (12% reduction) spec/selenium/grades/speedgrader 14:09 -> 13:13 (6.7% reduction), slowest spec went from 0:26 -> 0:19 speed gains in the app will likely be more modest, but performance should noticeably improve in places like the speedgrader (e.g. grade quiz by question) test plan: * specs * regression test of notifications throughout the app, in particular: * discussion topics * assignments * submissions Change-Id: I1ccf6f7f293a35763fde077d612505069823390e Reviewed-on: https://gerrit.instructure.com/102868 Reviewed-by: Landon Wilkins <lwilkins@instructure.com> Reviewed-by: Cody Cutrer <cody@instructure.com> Tested-by: Jenkins QA-Review: Cemal Aktas <caktas@instructure.com> Product-Review: Jon Jensen <jon@instructure.com>
diff --git a/app/controllers/discussion_topics_controller.rb b/app/controllers/discussion_topics_controller.rb
@@ -921,16 +921,17 @@ def process_discussion_topic(is_new = false)
     model_type = value_to_boolean(params[:is_announcement]) && @context.announcements.temp_record.grants_right?(@current_user, session, :create) ? :announcements : :discussion_topics
     if is_new
       @topic = @context.send(model_type).build
+      prior_version = @topic.dup
     else
       @topic = @context.send(model_type).active.find(params[:id] || params[:topic_id])
+      prior_version = DiscussionTopic.find(@topic.id)
     end
 
     return unless authorized_action(@topic, @current_user, (is_new ? :create : :update))
 
     allowed_fields = @context.is_a?(Group) ? API_ALLOWED_TOPIC_FIELDS_FOR_GROUP : API_ALLOWED_TOPIC_FIELDS
     discussion_topic_hash = params.permit(*allowed_fields)
 
-    prior_version = @topic.generate_prior_version
     process_podcast_parameters(discussion_topic_hash)
 
     if is_new
@@ -982,9 +983,7 @@ def process_discussion_topic(is_new = false)
         end
 
         @topic = DiscussionTopic.find(@topic.id)
-        @topic.just_created = is_new
-        @topic.prior_version = prior_version
-        @topic.broadcast_notifications
+        @topic.broadcast_notifications(prior_version)
 
         render :json => discussion_topic_api_json(@topic, @context, @current_user, session)
       else
diff --git a/app/models/assignment.rb b/app/models/assignment.rb
@@ -621,15 +621,12 @@ def touch_submissions_if_muted_changed
   # call this to perform notifications on an Assignment that is not being saved
   # (useful when a batch of overrides associated with a new assignment have been saved)
   def do_notifications!(prior_version=nil, notify=false)
-    self.prior_version = prior_version
-    @broadcasted = false
     # TODO: this will blow up if the group_category string is set on the
     # previous version, because it gets confused between the db string field
     # and the association.  one more reason to drop the db column
-    self.prior_version ||= self.versions.previous(self.current_version.number).try(:model)
-    self.just_created = self.prior_version.nil?
+    prior_version ||= self.versions.previous(self.current_version.number).try(:model)
     self.notify_of_update = notify || false
-    broadcast_notifications
+    broadcast_notifications(prior_version || dup)
     remove_assignment_updated_flag
   end
 
diff --git a/app/models/assignment_override.rb b/app/models/assignment_override.rb
@@ -285,10 +285,9 @@ def notify_change?
     self.assignment.context.available? &&
     self.assignment.published? &&
     self.assignment.created_at < 3.hours.ago &&
-    (!self.prior_version ||
-      self.workflow_state != self.prior_version.workflow_state ||
-      self.due_at_overridden != self.prior_version.due_at_overridden ||
-      self.due_at_overridden && !Assignment.due_dates_equal?(self.due_at, self.prior_version.due_at))
+    (workflow_state_changed? ||
+      due_at_overridden_changed? ||
+      due_at_overridden && !Assignment.due_dates_equal?(due_at, due_at_was))
   end
 
   def set_title_if_needed
diff --git a/app/models/broadcast_policies/assignment_policy.rb b/app/models/broadcast_policies/assignment_policy.rb
@@ -11,7 +11,7 @@ def should_dispatch_assignment_due_date_changed?
       accepting_messages? &&
       assignment.changed_in_state(:published, :fields => :due_at) &&
       !just_published? &&
-      !AssignmentPolicy.due_dates_equal?(assignment.due_at, prior_version.due_at) &&
+      !AssignmentPolicy.due_dates_equal?(assignment.due_at, assignment.due_at_was) &&
       created_before(3.hours.ago)
     end
 
@@ -21,14 +21,14 @@ def should_dispatch_assignment_changed?
       !assignment.muted? &&
       created_before(30.minutes.ago) &&
       !just_published? &&
-      (assignment.points_possible != prior_version.points_possible || assignment.assignment_changed)
+      (assignment.points_possible_changed? || assignment.assignment_changed)
     end
 
     def should_dispatch_assignment_created?
       return false unless context_sendable?
 
       published_on_create? || just_published? ||
-        (prior_version && prior_version.workflow_state != assignment.workflow_state && assignment.published?)
+        (assignment.workflow_state_changed? && assignment.published?)
     end
 
     def should_dispatch_assignment_unmuted?
@@ -45,11 +45,7 @@ def context_sendable?
 
     def accepting_messages?
       context_sendable? &&
-      prior_version
-    end
-
-    def prior_version
-      assignment.prior_version
+        !assignment.just_created
     end
 
     def created_before(time)
diff --git a/app/models/broadcast_policies/submission_policy.rb b/app/models/broadcast_policies/submission_policy.rb
@@ -78,16 +78,12 @@ def just_submitted?
     end
 
     def just_submitted_late?
-      (just_submitted? || prior.try(:submitted_at) != submission.submitted_at)
-    end
-
-    def prior
-      submission.prior_version
+      (just_submitted? || submission.submitted_at_changed?)
     end
 
     def is_a_resubmission?
-      prior.submitted_at &&
-      prior.submitted_at != submission.submitted_at
+      submission.submitted_at_was &&
+      submission.submitted_at_changed?
     end
 
     def grade_updated?
diff --git a/app/models/broadcast_policies/wiki_page_policy.rb b/app/models/broadcast_policies/wiki_page_policy.rb
@@ -22,7 +22,7 @@ def created_before?(time)
     def changed_while_published?
       wiki_page.published? &&
         wiki_page.wiki_page_changed &&
-        wiki_page.prior_version
+        !wiki_page.just_created
     end
   end
 end
diff --git a/app/models/conversation_message.rb b/app/models/conversation_message.rb
@@ -109,9 +109,7 @@ def self.preload_latest(conversation_participants, author=nil)
 
   def after_participants_created_broadcast
     conversation_message_participants(true) # reload this association so we get latest data
-    skip_broadcasts = false
     @re_send_message = true
-    set_broadcast_flags
     broadcast_notifications
     queue_create_stream_items
     generate_user_note!
diff --git a/app/models/course.rb b/app/models/course.rb
@@ -670,19 +670,15 @@ def potential_collaborators
     p.to { participating_students_by_date + participating_observers_by_date }
     p.whenever { |record|
       (record.available? && @grade_weight_changed) ||
-      (
-        record.prior_version.present? &&
-        record.changed_in_state(:available, :fields => :group_weighting_scheme)
-      )
+      record.changed_in_state(:available, :fields => :group_weighting_scheme)
     }
 
     p.dispatch :new_course
     p.to { self.root_account.account_users }
     p.whenever { |record|
       record.root_account &&
       ((record.just_created && record.name != Course.default_name) ||
-       (record.prior_version.present? &&
-         record.prior_version.name == Course.default_name &&
+       (record.name_was == Course.default_name &&
          record.name != Course.default_name)
       )
     }
diff --git a/app/models/submission.rb b/app/models/submission.rb
@@ -1391,7 +1391,7 @@ def <=>(other)
   end
 
   def assignment_graded_in_the_last_hour?
-    self.prior_version && self.prior_version.graded_at && self.prior_version.graded_at > 1.hour.ago
+    graded_at_was && graded_at_was > 1.hour.ago
   end
 
   def teacher
diff --git a/gems/broadcast_policy/lib/broadcast_policy.rb b/gems/broadcast_policy/lib/broadcast_policy.rb
@@ -33,14 +33,14 @@
 # set_broadcast_policy do
 #   dispatch :new_assignment
 #   to { self.students }
-#   whenever { |record| record.just_created? }
+#   whenever { |record| record.just_created }
 # end
 #
 # set_broadcast_policy do
 #   dispatch :assignment_change
 #   to { self.students }
 #   whenever { |record|
-#     record.prior_version != self.version and true
+#     record.workflow_state_changed?
 #     # ... some field-wise comparison
 #   }
 # end
diff --git a/gems/broadcast_policy/lib/broadcast_policy/class_methods.rb b/gems/broadcast_policy/lib/broadcast_policy/class_methods.rb
@@ -20,8 +20,7 @@ module ClassMethods #:nodoc:
     def has_a_broadcast_policy
       extend BroadcastPolicy::SingletonMethods
       include BroadcastPolicy::InstanceMethods
-      after_save :broadcast_notifications # Must be defined locally...
-      before_save :set_broadcast_flags
+      after_save :broadcast_notifications
     end
   end
 end
diff --git a/gems/broadcast_policy/lib/broadcast_policy/instance_methods.rb b/gems/broadcast_policy/lib/broadcast_policy/instance_methods.rb
@@ -1,5 +1,7 @@
+require "active_support/hash_with_indifferent_access"
+
 #
-# Copyright (C) 2011 - 2014 Instructure, Inc.
+# Copyright (C) 2011 - 2017 Instructure, Inc.
 #
 # This file is part of Canvas.
 #
@@ -18,8 +20,11 @@
 module BroadcastPolicy
   module InstanceMethods
 
-    # Some generic flags for inside the policy
-    attr_accessor :just_created, :prior_version
+    # either we're in an after_save of a new record, or we just
+    # finished saving one
+    def just_created
+      changed? ? id_was.nil? : previous_changes.key?(:id) && previous_changes[:id].first.nil?
+    end
 
     # Some flags for auditing policy matching
     def messages_sent
@@ -36,73 +41,69 @@ def messages_failed
       @messages_failed ||= {}
     end
 
-    # This is called before_save
-    def set_broadcast_flags
-      @broadcasted = false
-      unless @skip_broadcasts
-        self.just_created = self.new_record?
-        self.prior_version = generate_prior_version
-      end
-    end
-
-    def generate_prior_version
-      obj = self.class.new
-      self.attributes.each do |attr, value|
-        next unless obj.has_attribute?(attr)
-        value = changed_attributes[attr] if changed_attributes.key?(attr)
-        obj.write_attribute(attr, value)
+    def with_changed_attributes_from(other)
+      return yield unless other
+      begin
+        frd_changed_attributes = @changed_attributes
+        @changed_attributes = ActiveSupport::HashWithIndifferentAccess.new
+        other.attributes.each do |key, value|
+          @changed_attributes[key] = value if value != attributes[key]
+        end
+        yield
+      ensure
+        @changed_attributes = frd_changed_attributes
       end
-      obj
     end
 
-    # This is called after_save
-    def broadcast_notifications
-      return if @broadcasted
-      @broadcasted = true
+    # This is called after_save, but you can call it manually to trigger
+    # notifications. If you pass in a prior_version of self, its
+    # attributes will be used to fake out the various _was/_changed?
+    # helpers
+    def broadcast_notifications(prior_version = nil)
       raise ArgumentError, "Broadcast Policy block not supplied for #{self.class}" unless self.class.broadcast_policy_list
-      self.class.broadcast_policy_list.broadcast(self)
+      if prior_version
+        with_changed_attributes_from(prior_version) do
+          self.class.broadcast_policy_list.broadcast(self)
+        end
+      else
+        self.class.broadcast_policy_list.broadcast(self)
+      end
     end
 
     attr_accessor :skip_broadcasts
 
     def save_without_broadcasting
-      begin
-        @skip_broadcasts = true
-        self.save
-      ensure
-        @skip_broadcasts = false
-      end
+      @skip_broadcasts = true
+      self.save
+    ensure
+      @skip_broadcasts = false
     end
 
     def save_without_broadcasting!
-      begin
-        @skip_broadcasts = true
-        self.save!
-      ensure
-        @skip_broadcasts = false
-      end
+      @skip_broadcasts = true
+      self.save!
+    ensure
+      @skip_broadcasts = false
     end
 
     # The rest of the methods here should just be helper methods to make
     # writing a condition that much easier.
-    def changed_in_state(state, opts={})
-      fields  = opts[:fields] || []
-      fields = [fields] unless fields.is_a?(Array)
-
-      fields.map {|field| self.prior_version.send(field) != self.send(field) }.include?(true) &&
-        self.workflow_state == state.to_s &&
-        self.prior_version.workflow_state == state.to_s
+    def changed_in_state(state, fields: [])
+      fields = Array(fields)
+      fields.any? { |field| attribute_changed?(field) } &&
+        workflow_state == state.to_s &&
+        workflow_state_was == state.to_s
     end
 
     def changed_state(new_state=nil, old_state=nil)
       if new_state && old_state
-        self.workflow_state == new_state.to_s &&
-          self.prior_version.workflow_state == old_state.to_s
+        workflow_state == new_state.to_s &&
+          workflow_state_was == old_state.to_s
       elsif new_state
-        self.workflow_state.to_s == new_state.to_s &&
-          self.prior_version.workflow_state != self.workflow_state
+        workflow_state.to_s == new_state.to_s &&
+          workflow_state_changed?
       else
-        self.workflow_state != self.prior_version.workflow_state
+        workflow_state_changed?
       end
     end
     alias :changed_state_to :changed_state
diff --git a/gems/broadcast_policy/spec/broadcast_policy/instance_methods_spec.rb b/gems/broadcast_policy/spec/broadcast_policy/instance_methods_spec.rb
diff --git a/spec/models/broadcast_policies/assignment_policy_spec.rb b/spec/models/broadcast_policies/assignment_policy_spec.rb
diff --git a/spec/models/broadcast_policies/submission_policy_spec.rb b/spec/models/broadcast_policies/submission_policy_spec.rb
diff --git a/spec/models/broadcast_policies/wiki_page_policy_spec.rb b/spec/models/broadcast_policies/wiki_page_policy_spec.rb
diff --git a/spec/models/course_spec.rb b/spec/models/course_spec.rb
