Merge branch 'fj-37736-improve-performance-post-receive-cleaning-code' into 'master'

Code cleaning in PostReceive services

See merge request gitlab-org/gitlab-ce!20916

4 files changed, 59 insertions, 44 deletions

diff --git a/app/models/repository.rb b/app/models/repository.rb
index 9873d9a6327..50f42be661b 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -312,6 +312,8 @@ class Repository
   # types - An Array of file types (e.g. `:readme`) used to refresh extra
   #         caches.
   def refresh_method_caches(types)
+    return if types.empty?
+
     to_refresh = []
 
     types.each do |type|
diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb
index f0587667d37..e3640e38bfa 100644
--- a/app/services/git_push_service.rb
+++ b/app/services/git_push_service.rb
@@ -3,6 +3,7 @@
 class GitPushService < BaseService
   attr_accessor :push_data, :push_commits
   include Gitlab::Access
+  include Gitlab::Utils::StrongMemoize
 
   # The N most recent commits to process in a single push payload.
   PROCESS_COMMIT_LIMIT = 100
@@ -21,14 +22,14 @@ class GitPushService < BaseService
   #  6. Checks if the project's main language has changed
   #
   def execute
-    @project.repository.after_create if @project.empty_repo?
-    @project.repository.after_push_commit(branch_name)
+    project.repository.after_create if project.empty_repo?
+    project.repository.after_push_commit(branch_name)
 
     if push_remove_branch?
-      @project.repository.after_remove_branch
+      project.repository.after_remove_branch
       @push_commits = []
     elsif push_to_new_branch?
-      @project.repository.after_create_branch
+      project.repository.after_create_branch
 
       # Re-find the pushed commits.
       if default_branch?
@@ -38,14 +39,14 @@ class GitPushService < BaseService
         # Use the pushed commits that aren't reachable by the default branch
         # as a heuristic. This may include more commits than are actually pushed, but
         # that shouldn't matter because we check for existing cross-references later.
-        @push_commits = @project.repository.commits_between(@project.default_branch, params[:newrev])
+        @push_commits = project.repository.commits_between(project.default_branch, params[:newrev])
 
         # don't process commits for the initial push to the default branch
         process_commit_messages
       end
     elsif push_to_existing_branch?
       # Collect data for this git push
-      @push_commits = @project.repository.commits_between(params[:oldrev], params[:newrev])
+      @push_commits = project.repository.commits_between(params[:oldrev], params[:newrev])
 
       process_commit_messages
 
@@ -64,7 +65,7 @@ class GitPushService < BaseService
   end
 
   def update_gitattributes
-    @project.repository.copy_gitattributes(params[:ref])
+    project.repository.copy_gitattributes(params[:ref])
   end
 
   def update_caches
@@ -88,7 +89,7 @@ class GitPushService < BaseService
       types = []
     end
 
-    ProjectCacheWorker.perform_async(@project.id, types, [:commit_count, :repository_size])
+    ProjectCacheWorker.perform_async(project.id, types, [:commit_count, :repository_size])
   end
 
   def update_signatures
@@ -121,10 +122,10 @@ class GitPushService < BaseService
   protected
 
   def update_remote_mirrors
-    return unless @project.has_remote_mirror?
+    return unless project.has_remote_mirror?
 
-    @project.mark_stuck_remote_mirrors_as_failed!
-    @project.update_remote_mirrors
+    project.mark_stuck_remote_mirrors_as_failed!
+    project.update_remote_mirrors
   end
 
   def execute_related_hooks
@@ -132,14 +133,14 @@ class GitPushService < BaseService
     # could cause the last commit of a merge request to change.
     #
     UpdateMergeRequestsWorker
-      .perform_async(@project.id, current_user.id, params[:oldrev], params[:newrev], params[:ref])
+      .perform_async(project.id, current_user.id, params[:oldrev], params[:newrev], params[:ref])
 
-    EventCreateService.new.push(@project, current_user, build_push_data)
-    Ci::CreatePipelineService.new(@project, current_user, build_push_data).execute(:push)
+    EventCreateService.new.push(project, current_user, build_push_data)
+    Ci::CreatePipelineService.new(project, current_user, build_push_data).execute(:push)
 
     SystemHookPushWorker.perform_async(build_push_data.dup, :push_hooks)
-    @project.execute_hooks(build_push_data.dup, :push_hooks)
-    @project.execute_services(build_push_data.dup, :push_hooks)
+    project.execute_hooks(build_push_data.dup, :push_hooks)
+    project.execute_services(build_push_data.dup, :push_hooks)
 
     if push_remove_branch?
       AfterBranchDeleteService
@@ -149,52 +150,50 @@ class GitPushService < BaseService
   end
 
   def perform_housekeeping
-    housekeeping = Projects::HousekeepingService.new(@project)
+    housekeeping = Projects::HousekeepingService.new(project)
     housekeeping.increment!
     housekeeping.execute if housekeeping.needed?
   rescue Projects::HousekeepingService::LeaseTaken
   end
 
   def process_default_branch
-    @push_commits_count = project.repository.commit_count_for_ref(params[:ref])
-
-    offset = [@push_commits_count - PROCESS_COMMIT_LIMIT, 0].max
+    offset = [push_commits_count - PROCESS_COMMIT_LIMIT, 0].max
     @push_commits = project.repository.commits(params[:newrev], offset: offset, limit: PROCESS_COMMIT_LIMIT)
 
-    @project.after_create_default_branch
+    project.after_create_default_branch
   end
 
   def build_push_data
     @push_data ||= Gitlab::DataBuilder::Push.build(
-      @project,
+      project,
       current_user,
       params[:oldrev],
       params[:newrev],
       params[:ref],
       @push_commits,
-      commits_count: @push_commits_count)
+      commits_count: push_commits_count)
   end
 
   def push_to_existing_branch?
     # Return if this is not a push to a branch (e.g. new commits)
-    Gitlab::Git.branch_ref?(params[:ref]) && !Gitlab::Git.blank_ref?(params[:oldrev])
+    branch_ref? && !Gitlab::Git.blank_ref?(params[:oldrev])
   end
 
   def push_to_new_branch?
-    Gitlab::Git.branch_ref?(params[:ref]) && Gitlab::Git.blank_ref?(params[:oldrev])
+    strong_memoize(:push_to_new_branch) do
+      branch_ref? && Gitlab::Git.blank_ref?(params[:oldrev])
+    end
   end
 
   def push_remove_branch?
-    Gitlab::Git.branch_ref?(params[:ref]) && Gitlab::Git.blank_ref?(params[:newrev])
-  end
-
-  def push_to_branch?
-    Gitlab::Git.branch_ref?(params[:ref])
+    strong_memoize(:push_remove_branch) do
+      branch_ref? && Gitlab::Git.blank_ref?(params[:newrev])
+    end
   end
 
   def default_branch?
-    Gitlab::Git.branch_ref?(params[:ref]) &&
-      (Gitlab::Git.ref_name(params[:ref]) == project.default_branch || project.default_branch.nil?)
+    branch_ref? &&
+      (branch_name == project.default_branch || project.default_branch.nil?)
   end
 
   def commit_user(commit)
@@ -202,7 +201,21 @@ class GitPushService < BaseService
   end
 
   def branch_name
-    @branch_name ||= Gitlab::Git.ref_name(params[:ref])
+    strong_memoize(:branch_name) do
+      Gitlab::Git.ref_name(params[:ref])
+    end
+  end
+
+  def branch_ref?
+    strong_memoize(:branch_ref) do
+      Gitlab::Git.branch_ref?(params[:ref])
+    end
+  end
+
+  def push_commits_count
+    strong_memoize(:push_commits_count) do
+      project.repository.commit_count_for_ref(params[:ref])
+    end
   end
 
   def last_pushed_commits
diff --git a/app/services/git_tag_push_service.rb b/app/services/git_tag_push_service.rb
index 3ff2d1d107d..dbadafc0f52 100644
--- a/app/services/git_tag_push_service.rb
+++ b/app/services/git_tag_push_service.rb
@@ -9,12 +9,12 @@ class GitTagPushService < BaseService
 
     @push_data = build_push_data
 
-    EventCreateService.new.push(project, current_user, @push_data)
-    Ci::CreatePipelineService.new(project, current_user, @push_data).execute(:push)
+    EventCreateService.new.push(project, current_user, push_data)
+    Ci::CreatePipelineService.new(project, current_user, push_data).execute(:push)
 
-    SystemHooksService.new.execute_hooks(build_system_push_data.dup, :tag_push_hooks)
-    project.execute_hooks(@push_data.dup, :tag_push_hooks)
-    project.execute_services(@push_data.dup, :tag_push_hooks)
+    SystemHooksService.new.execute_hooks(build_system_push_data, :tag_push_hooks)
+    project.execute_hooks(push_data.dup, :tag_push_hooks)
+    project.execute_services(push_data.dup, :tag_push_hooks)
 
     ProjectCacheWorker.perform_async(project.id, [], [:commit_count, :repository_size])
 
diff --git a/lib/gitlab/git_post_receive.rb b/lib/gitlab/git_post_receive.rb
index 742118b76a8..e731e654f3c 100644
--- a/lib/gitlab/git_post_receive.rb
+++ b/lib/gitlab/git_post_receive.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module Gitlab
   class GitPostReceive
     include Gitlab::Identifier
@@ -14,10 +16,11 @@ module Gitlab
     end
 
     def changes_refs
-      return enum_for(:changes_refs) unless block_given?
+      return changes unless block_given?
 
       changes.each do |change|
-        oldrev, newrev, ref = change.strip.split(' ')
+        change.strip!
+        oldrev, newrev, ref = change.split(' ')
 
         yield oldrev, newrev, ref
       end
@@ -26,13 +29,10 @@ module Gitlab
     private
 
     def deserialize_changes(changes)
-      changes = utf8_encode_changes(changes)
-      changes.lines
+      utf8_encode_changes(changes).each_line
     end
 
     def utf8_encode_changes(changes)
-      changes = changes.dup
-
       changes.force_encoding('UTF-8')
       return changes if changes.valid_encoding?