diff options
author | Sean McGivern <sean@gitlab.com> | 2017-10-04 13:43:39 +0100 |
---|---|---|
committer | Sean McGivern <sean@gitlab.com> | 2017-10-04 13:50:05 +0100 |
commit | d7717192782232c14c4f16962ab450b08348a4c9 (patch) | |
tree | e58691d259a9dc49065a9487f7036718339d902a /lib/declarative_policy | |
parent | 4a4f809353a2e7007f8c6d33bfb1e4d09ed5a560 (diff) | |
download | gitlab-ce-d7717192782232c14c4f16962ab450b08348a4c9.tar.gz |
Speed up DeclarativePolicy::Runner#steps_by_score
There were a couple of things here:
1. If the state was already enabled, we don't need to check all the remaining
steps - only those that can prevent the state. (An enable followed by an
enable is a no-op.) This logic is in `#run`, but we still did the work of
scoring and sorting the steps.
2. The sorting is known to be inefficient, but we can make it slightly more
efficient by stopping once we have a step with zero score, as that means it's
free.
Neither of these make this _fast_, especially when called lots of times - as we
do when there is lots of activity on an issue - but they do help some.
Diffstat (limited to 'lib/declarative_policy')
-rw-r--r-- | lib/declarative_policy/runner.rb | 31 |
1 files changed, 20 insertions, 11 deletions
diff --git a/lib/declarative_policy/runner.rb b/lib/declarative_policy/runner.rb index 56afd1f1392..45ff2ef9ced 100644 --- a/lib/declarative_policy/runner.rb +++ b/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 |