Merge branch 'perf/policy-class-cache' into 'master'

Perf: Improve performance of basic ruby ops in DeclarativePolicy

See merge request !12922

3 files changed, 51 insertions, 17 deletions

diff --git a/lib/declarative_policy.rb b/lib/declarative_policy.rb
index d9959bc1aff..b1eb1a6cef1 100644
--- a/lib/declarative_policy.rb
+++ b/lib/declarative_policy.rb
@@ -8,7 +8,12 @@ require_dependency 'declarative_policy/step'
 
 require_dependency 'declarative_policy/base'
 
+require 'thread'
+
 module DeclarativePolicy
+  CLASS_CACHE_MUTEX = Mutex.new
+  CLASS_CACHE_IVAR = :@__DeclarativePolicy_CLASS_CACHE
+
   class << self
     def policy_for(user, subject, opts = {})
       cache = opts[:cache] || {}
@@ -23,7 +28,36 @@ module DeclarativePolicy
 
       subject = find_delegate(subject)
 
-      subject.class.ancestors.each do |klass|
+      class_for_class(subject.class)
+    end
+
+    private
+
+    # This method is heavily cached because there are a lot of anonymous
+    # modules in play in a typical rails app, and #name performs quite
+    # slowly for anonymous classes and modules.
+    #
+    # See https://bugs.ruby-lang.org/issues/11119
+    #
+    # if the above bug is resolved, this caching could likely be removed.
+    def class_for_class(subject_class)
+      unless subject_class.instance_variable_defined?(CLASS_CACHE_IVAR)
+        CLASS_CACHE_MUTEX.synchronize do
+          # re-check in case of a race
+          break if subject_class.instance_variable_defined?(CLASS_CACHE_IVAR)
+
+          policy_class = compute_class_for_class(subject_class)
+          subject_class.instance_variable_set(CLASS_CACHE_IVAR, policy_class)
+        end
+      end
+
+      policy_class = subject_class.instance_variable_get(CLASS_CACHE_IVAR)
+      raise "no policy for #{subject.class.name}" if policy_class.nil?
+      policy_class
+    end
+
+    def compute_class_for_class(subject_class)
+      subject_class.ancestors.each do |klass|
         next unless klass.name
 
         begin
@@ -37,12 +71,8 @@ module DeclarativePolicy
           nil
         end
       end
-
-      raise "no policy for #{subject.class.name}"
     end
 
-    private
-
     def find_delegate(subject)
       seen = Set.new
 
diff --git a/lib/declarative_policy/cache.rb b/lib/declarative_policy/cache.rb
index b8cc60074c7..0804edba016 100644
--- a/lib/declarative_policy/cache.rb
+++ b/lib/declarative_policy/cache.rb
@@ -21,11 +21,14 @@ module DeclarativePolicy
       private
 
       def id_for(obj)
-        if obj.respond_to?(:id) && obj.id
-          obj.id.to_s
-        else
-          "##{obj.object_id}"
-        end
+        id =
+          begin
+            obj.id
+          rescue NoMethodError
+            nil
+          end
+
+        id || "##{obj.object_id}"
       end
     end
   end
diff --git a/lib/declarative_policy/condition.rb b/lib/declarative_policy/condition.rb
index 9d7cf6b9726..51c4a8b2bbe 100644
--- a/lib/declarative_policy/condition.rb
+++ b/lib/declarative_policy/condition.rb
@@ -82,13 +82,14 @@ module DeclarativePolicy
     # depending on the scope, we may cache only by the user or only by
     # the subject, resulting in sharing across different policy objects.
     def cache_key
-      case @condition.scope
-      when :normal  then "/dp/condition/#{@condition.key}/#{user_key},#{subject_key}"
-      when :user    then "/dp/condition/#{@condition.key}/#{user_key}"
-      when :subject then "/dp/condition/#{@condition.key}/#{subject_key}"
-      when :global  then "/dp/condition/#{@condition.key}"
-      else raise 'invalid scope'
-      end
+      @cache_key ||=
+        case @condition.scope
+        when :normal  then "/dp/condition/#{@condition.key}/#{user_key},#{subject_key}"
+        when :user    then "/dp/condition/#{@condition.key}/#{user_key}"
+        when :subject then "/dp/condition/#{@condition.key}/#{subject_key}"
+        when :global  then "/dp/condition/#{@condition.key}"
+        else raise 'invalid scope'
+        end
     end
 
     def user_key