Use update_columns to by_pass all the dirty code on active_record18663-avoid-extra-yaml-serializations

We think this is valid because we’re just storing new data that always will be valid

2 files changed, 38 insertions, 10 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 5c735bd1420..8f5fa9bc698 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -11,6 +11,7 @@ v 8.10.0 (unreleased)
   - Fix check for New Branch button on Issue page !4630 (winniehell)
   - Fix MR-auto-close text added to description. !4836
   - Fix pagination when sorting by columns with lots of ties (like priority)
+  - MergeRequestDiff reload content use update_columns to avoid multiple YAML de/serializations
   - Exclude email check from the standard health check
   - Fix changing issue state columns in milestone view
   - Fix user creation with stronger minimum password requirements !4054 (nathan-pmt)
diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb
index aca377cc600..a75fcb4c4f6 100644
--- a/app/models/merge_request_diff.rb
+++ b/app/models/merge_request_diff.rb
@@ -108,44 +108,46 @@ class MergeRequestDiff < ActiveRecord::Base
   # Reload all commits related to current merge request from repo
   # and save it as array of hashes in st_commits db field
   def reload_commits
+    new_attributes = {}
+
     commit_objects = unmerged_commits
 
     if commit_objects.present?
-      self.st_commits = dump_commits(commit_objects)
+      new_attributes[:st_commits] = dump_commits(commit_objects)
     end
 
-    save
+    update_columns_serialized(new_attributes)
   end
 
   # Reload diffs between branches related to current merge request from repo
   # and save it as array of hashes in st_diffs db field
   def reload_diffs
+    new_attributes = {}
     new_diffs = []
 
     if commits.size.zero?
-      self.state = :empty
+      new_attributes[:state] = :empty
     else
       diff_collection = unmerged_diffs
 
       if diff_collection.overflow?
         # Set our state to 'overflow' to make the #empty? and #collected?
         # methods (generated by StateMachine) return false.
-        self.state = :overflow
+        new_attributes[:state] = :overflow
       end
 
-      self.real_size = diff_collection.real_size
+      new_attributes[:real_size] = diff_collection.real_size
 
       if diff_collection.any?
         new_diffs = dump_diffs(diff_collection)
-        self.state = :collected
+        new_attributes[:state] = :collected
       end
     end
 
-    self.st_diffs = new_diffs
-
-    self.base_commit_sha = self.repository.merge_base(self.head, self.base)
+    new_attributes[:st_diffs] = new_diffs
+    new_attributes[:base_commit_sha] = self.repository.merge_base(self.head, self.base)
 
-    self.save
+    update_columns_serialized(new_attributes)
   end
 
   # Collect array of Git::Diff objects
@@ -190,4 +192,29 @@ class MergeRequestDiff < ActiveRecord::Base
         )
       end
   end
+
+  private
+
+  #
+  # #save or #update_attributes providing changes on serialized attributes do a lot of
+  # serialization and deserialization calls with a bad performance.
+  # Using #update_columns solve the problem with just one YAML.dump per serialized attribute that we provide.
+  # As a tradeoff we need to reload the current instance to properly manage time objects on those serialized
+  # attributes. So to keep same behaviour than the attribute assignment we reload the instance variable.
+  # The difference is in the usage of
+  # #write_attribute= (#update_attributes) and #raw_write_attribute= (#update_columns)
+  #
+  # Ex:
+  #
+  #   new_attributes[:st_commits].first.slice(:committed_date)
+  #   => {:committed_date=>2014-02-27 11:01:38 +0200}
+  #   YAML.load(YAML.dump(new_attributes[:st_commits].first.slice(:committed_date)))
+  #   => {:committed_date=>2014-02-27 10:01:38 +0100}
+  #
+  def update_columns_serialized(new_attributes)
+    return unless new_attributes.any?
+
+    update_columns(new_attributes)
+    reload
+  end
 end