Merge branch 'declarative-policy-optimisations' into 'master'

Speed up DeclarativePolicy::Runner#steps_by_score

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

changelogs/unreleased/declarative-policy-optimisations.yml

+---
+title: Speed up permission checks
+merge_request:
+author:
+type: other

lib/declarative_policy/rule.rb

@@ -206,11 +206,13 @@ module DeclarativePolicy
       end
 
       def cached_pass?(context)
-        passes = @rules.map { |r| r.cached_pass?(context) }
-        return false if passes.any? { |p| p == false }
-        return true if passes.all? { |p| p == true }
+        @rules.each do |rule|
+          pass = rule.cached_pass?(context)
 
-        nil
+          return pass if pass.nil? || pass == false
+        end
+
+        true
       end
 
       def repr
@@ -245,11 +247,13 @@ module DeclarativePolicy
       end
 
       def cached_pass?(context)
-        passes = @rules.map { |r| r.cached_pass?(context) }
-        return true if passes.any? { |p| p == true }
-        return false if passes.all? { |p| p == false }
+        @rules.each do |rule|
+          pass = rule.cached_pass?(context)
 
-        nil
+          return pass if pass.nil? || pass == true
+        end
+
+        false
       end
 
       def score(context)

lib/declarative_policy/runner.rb

@@ -107,7 +107,7 @@ module DeclarativePolicy
     end
 
     # This is the core spot where all those `#score` methods matter.
-    # It is critcal for performance to run steps in the correct order,
+    # It is critical for performance to run steps in the correct order,
     # so that we don't compute expensive conditions (potentially n times
     # if we're called on, say, a large list of users).
     #
@@ -139,30 +139,39 @@ module DeclarativePolicy
         return
       end
 
-      steps = Set.new(@steps)
-      remaining_enablers = steps.count { |s| s.enable? }
+      remaining_steps = Set.new(@steps)
+      remaining_enablers, remaining_preventers = remaining_steps.partition(&:enable?).map { |s| Set.new(s) }
 
       loop do
-        return if steps.empty?
+        if @state.enabled?
+          # Once we set this, we never need to unset it, because a single
+          # prevent will stop this from being enabled
+          remaining_steps = remaining_preventers
+        else
+          # if the permission hasn't yet been enabled and we only have
+          # prevent steps left, we short-circuit the state here
+          @state.prevent! if remaining_enablers.empty?
+        end
 
-        # if the permission hasn't yet been enabled and we only have
-        # prevent steps left, we short-circuit the state here
-        @state.prevent! if !@state.enabled? && remaining_enablers == 0
+        return if remaining_steps.empty?
 
         lowest_score = Float::INFINITY
         next_step = nil
 
-        steps.each do |step|
+        remaining_steps.each do |step|
           score = step.score
+
           if score < lowest_score
             next_step = step
             lowest_score = score
           end
-        end
 
-        steps.delete(next_step)
+          break if lowest_score.zero?
+        end
 
-        remaining_enablers -= 1 if next_step.enable?
+        [remaining_steps, remaining_enablers, remaining_preventers].each do |set|
+          set.delete(next_step)
+        end
 
         yield next_step, lowest_score
       end