Merge branch '41731-predicate-memoization' into 'master'

Introduce PredicateMemoization cop

Closes #41731

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

app/models/concerns/resolvable_discussion.rb

@@ -1,5 +1,6 @@
 module ResolvableDiscussion
   extend ActiveSupport::Concern
+  include ::Gitlab::Utils::StrongMemoize
 
   included do
     # A number of properties of this `Discussion`, like `first_note` and `resolvable?`, are memoized.
@@ -31,27 +32,37 @@ module ResolvableDiscussion
   end
 
   def resolvable?
-    @resolvable ||= potentially_resolvable? && notes.any?(&:resolvable?)
+    strong_memoize(:resolvable) do
+      potentially_resolvable? && notes.any?(&:resolvable?)
+    end
   end
 
   def resolved?
-    @resolved ||= resolvable? && notes.none?(&:to_be_resolved?)
+    strong_memoize(:resolved) do
+      resolvable? && notes.none?(&:to_be_resolved?)
+    end
   end
 
   def first_note
-    @first_note ||= notes.first
+    strong_memoize(:first_note) do
+      notes.first
+    end
   end
 
   def first_note_to_resolve
     return unless resolvable?
 
-    @first_note_to_resolve ||= notes.find(&:to_be_resolved?) # rubocop:disable Gitlab/ModuleWithInstanceVariables
+    strong_memoize(:first_note_to_resolve) do
+      notes.find(&:to_be_resolved?)
+    end
   end
 
   def last_resolved_note
     return unless resolved?
 
-    @last_resolved_note ||= resolved_notes.sort_by(&:resolved_at).last # rubocop:disable Gitlab/ModuleWithInstanceVariables
+    strong_memoize(:last_resolved_note) do
+      resolved_notes.sort_by(&:resolved_at).last
+    end
   end
 
   def resolved_notes
@@ -93,8 +104,8 @@ module ResolvableDiscussion
     # Set the notes array to the updated notes
     @notes = notes_relation.fresh.to_a # rubocop:disable Gitlab/ModuleWithInstanceVariables
 
-    self.class.memoized_values.each do |var|
-      instance_variable_set(:"@#{var}", nil)
+    self.class.memoized_values.each do |name|
+      clear_memoization(name)
     end
   end
 end

app/models/project.rb

@@ -20,6 +20,7 @@ class Project < ActiveRecord::Base
   include GroupDescendant
   include Gitlab::SQL::Pattern
   include DeploymentPlatform
+  include ::Gitlab::Utils::StrongMemoize
 
   extend Gitlab::ConfigHelper
   extend Gitlab::CurrentSettings
@@ -993,9 +994,13 @@ class Project < ActiveRecord::Base
   end
 
   def repo_exists?
-    @repo_exists ||= repository.exists?
-  rescue
-    @repo_exists = false
+    strong_memoize(:repo_exists) do
+      begin
+        repository.exists?
+      rescue
+        false
+      end
+    end
   end
 
   def root_ref?(branch)

app/models/repository.rb

@@ -895,15 +895,18 @@ class Repository
     branch = Gitlab::Git::Branch.find(self, branch_or_name)
 
     if branch
-      @root_ref_sha ||= commit(root_ref).sha
-      same_head = branch.target == @root_ref_sha
-      merged = ancestor?(branch.target, @root_ref_sha)
+      same_head = branch.target == root_ref_sha
+      merged = ancestor?(branch.target, root_ref_sha)
       !same_head && merged
     else
       nil
     end
   end
 
+  def root_ref_sha
+    @root_ref_sha ||= commit(root_ref).sha
+  end
+
   delegate :merged_branch_names, to: :raw_repository
 
   def merge_base(first_commit_id, second_commit_id)

changelogs/unreleased/41731-predicate-memoization.yml

@@ -0,0 +1,5 @@
+---
+title: Properly memoize some predicate methods
+merge_request: 16329
+author:
+type: performance

lib/gitlab/background_migration/prepare_untracked_uploads.rb

@@ -7,6 +7,7 @@ module Gitlab
     class PrepareUntrackedUploads # rubocop:disable Metrics/ClassLength
       # For bulk_queue_background_migration_jobs_by_range
       include Database::MigrationHelpers
+      include ::Gitlab::Utils::StrongMemoize
 
       FIND_BATCH_SIZE = 500
       RELATIVE_UPLOAD_DIR = "uploads".freeze
@@ -142,7 +143,9 @@ module Gitlab
       end
 
       def postgresql?
-        @postgresql ||= Gitlab::Database.postgresql?
+        strong_memoize(:postgresql) do
+          Gitlab::Database.postgresql?
+        end
       end
 
       def can_bulk_insert_and_ignore_duplicates?
@@ -150,8 +153,9 @@ module Gitlab
       end
 
       def postgresql_pre_9_5?
-        @postgresql_pre_9_5 ||= postgresql? &&
-          Gitlab::Database.version.to_f < 9.5
+        strong_memoize(:postgresql_pre_9_5) do
+          postgresql? && Gitlab::Database.version.to_f < 9.5
+        end
       end
 
       def schedule_populate_untracked_uploads_jobs

lib/gitlab/bare_repository_import/repository.rb

@@ -1,6 +1,8 @@
 module Gitlab
   module BareRepositoryImport
     class Repository
+      include ::Gitlab::Utils::StrongMemoize
+
       attr_reader :group_path, :project_name, :repo_path
 
       def initialize(root_path, repo_path)
@@ -41,11 +43,15 @@ module Gitlab
       private
 
       def wiki?
-        @wiki ||= repo_path.end_with?('.wiki.git')
+        strong_memoize(:wiki) do
+          repo_path.end_with?('.wiki.git')
+        end
       end
 
       def hashed?
-        @hashed ||= repo_relative_path.include?('@hashed')
+        strong_memoize(:hashed) do
+          repo_relative_path.include?('@hashed')
+        end
       end
 
       def repo_relative_path

lib/gitlab/ci/pipeline/chain/skip.rb

@@ -3,6 +3,8 @@ module Gitlab
     module Pipeline
       module Chain
         class Skip < Chain::Base
+          include ::Gitlab::Utils::StrongMemoize
+
           SKIP_PATTERN = /\[(ci[ _-]skip|skip[ _-]ci)\]/i
 
           def perform!
@@ -24,7 +26,9 @@ module Gitlab
           def commit_message_skips_ci?
             return false unless @pipeline.git_commit_message
 
-            @skipped ||= !!(@pipeline.git_commit_message =~ SKIP_PATTERN)
+            strong_memoize(:commit_message_skips_ci) do
+              !!(@pipeline.git_commit_message =~ SKIP_PATTERN)
+            end
           end
         end
       end

lib/gitlab/ci/stage/seed.rb

@@ -2,6 +2,8 @@ module Gitlab
   module Ci
     module Stage
       class Seed
+        include ::Gitlab::Utils::StrongMemoize
+
         attr_reader :pipeline
 
         delegate :project, to: :pipeline
@@ -50,7 +52,9 @@ module Gitlab
         private
 
         def protected_ref?
-          @protected_ref ||= project.protected_for?(pipeline.ref)
+          strong_memoize(:protected_ref) do
+            project.protected_for?(pipeline.ref)
+          end
         end
       end
     end

lib/gitlab/github_import/client.rb

@@ -14,6 +14,8 @@ module Gitlab
     #       puts label.name
     #     end
     class Client
+      include ::Gitlab::Utils::StrongMemoize
+
       attr_reader :octokit
 
       # A single page of data and the corresponding page number.
@@ -173,7 +175,9 @@ module Gitlab
       end
 
       def rate_limiting_enabled?
-        @rate_limiting_enabled ||= api_endpoint.include?('.github.com')
+        strong_memoize(:rate_limiting_enabled) do
+          api_endpoint.include?('.github.com')
+        end
       end
 
       def api_endpoint

lib/gitlab/user_access.rb

@@ -16,8 +16,10 @@ module Gitlab
     def can_do_action?(action)
       return false unless can_access_git?
 
-      @permission_cache ||= {}
-      @permission_cache[action] ||= user.can?(action, project)
+      permission_cache[action] =
+        permission_cache.fetch(action) do
+          user.can?(action, project)
+        end
     end
 
     def cannot_do_action?(action)
@@ -88,6 +90,10 @@ module Gitlab
 
     private
 
+    def permission_cache
+      @permission_cache ||= {}
+    end
+
     def can_access_git?
       user && user.can?(:access_git)
     end

rubocop/cop/gitlab/predicate_memoization.rb

@@ -0,0 +1,39 @@
+module RuboCop
+  module Cop
+    module Gitlab
+      class PredicateMemoization < RuboCop::Cop::Cop
+        MSG = <<~EOL.freeze
+          Avoid using `@value ||= query` inside predicate methods in order to
+          properly memoize `false` or `nil` values.
+          https://docs.gitlab.com/ee/development/utilities.html#strongmemoize
+        EOL
+
+        def on_def(node)
+          return unless predicate_method?(node)
+
+          select_offenses(node).each do |offense|
+            add_offense(offense, location: :expression)
+          end
+        end
+
+        private
+
+        def predicate_method?(node)
+          node.method_name.to_s.end_with?('?')
+        end
+
+        def or_ivar_assignment?(or_assignment)
+          lhs = or_assignment.each_child_node.first
+
+          lhs.ivasgn_type?
+        end
+
+        def select_offenses(node)
+          node.each_descendant(:or_asgn).select do |or_assignment|
+            or_ivar_assignment?(or_assignment)
+          end
+        end
+      end
+    end
+  end
+end

rubocop/rubocop.rb

@@ -1,4 +1,5 @@
 require_relative 'cop/gitlab/module_with_instance_variables'
+require_relative 'cop/gitlab/predicate_memoization'
 require_relative 'cop/include_sidekiq_worker'
 require_relative 'cop/line_break_around_conditional_block'
 require_relative 'cop/migration/add_column'

spec/rubocop/cop/gitlab/predicate_memoization_spec.rb

@@ -0,0 +1,100 @@
+require 'spec_helper'
+require 'rubocop'
+require 'rubocop/rspec/support'
+require_relative '../../../../rubocop/cop/gitlab/predicate_memoization'
+
+describe RuboCop::Cop::Gitlab::PredicateMemoization do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  shared_examples('registering offense') do |options|
+    let(:offending_lines) { options[:offending_lines] }
+
+    it 'registers an offense when a predicate method is memoizing via ivar' do
+      inspect_source(source)
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(offending_lines.size)
+        expect(cop.offenses.map(&:line)).to eq(offending_lines)
+      end
+    end
+  end
+
+  shared_examples('not registering offense') do
+    it 'does not register offenses' do
+      inspect_source(source)
+
+      expect(cop.offenses).to be_empty
+    end
+  end
+
+  context 'when source is a predicate method memoizing via ivar' do
+    it_behaves_like 'registering offense', offending_lines: [3] do
+      let(:source) do
+        <<~RUBY
+          class C
+            def really?
+              @really ||= true
+            end
+          end
+        RUBY
+      end
+    end
+
+    it_behaves_like 'registering offense', offending_lines: [4] do
+      let(:source) do
+        <<~RUBY
+          class C
+            def really?
+              value = true
+              @really ||= value
+            end
+          end
+        RUBY
+      end
+    end
+  end
+
+  context 'when source is a predicate method using ivar with assignment' do
+    it_behaves_like 'not registering offense' do
+      let(:source) do
+        <<~RUBY
+          class C
+            def really?
+              @really = true
+            end
+          end
+        RUBY
+      end
+    end
+  end
+
+  context 'when source is a predicate method using local with ||=' do
+    it_behaves_like 'not registering offense' do
+      let(:source) do
+        <<~RUBY
+          class C
+            def really?
+              really ||= true
+            end
+          end
+        RUBY
+      end
+    end
+  end
+
+  context 'when source is a regular method memoizing via ivar' do
+    it_behaves_like 'not registering offense' do
+      let(:source) do
+        <<~RUBY
+          class C
+            def really
+              @really ||= true
+            end
+          end
+        RUBY
+      end
+    end
+  end
+end