Merge pull request #8510 from thedarkone/thread_safety_improvements

Thread safety improvements

6 files changed, 48 insertions, 63 deletions

diff --git a/actionpack/lib/action_dispatch/http/filter_parameters.rb b/actionpack/lib/action_dispatch/http/filter_parameters.rb
index 4a7df6b657..4fea690862 100644
--- a/actionpack/lib/action_dispatch/http/filter_parameters.rb
+++ b/actionpack/lib/action_dispatch/http/filter_parameters.rb
@@ -1,4 +1,4 @@
-require 'mutex_m'
+require 'thread_safe'
 require 'active_support/core_ext/hash/keys'
 require 'active_support/core_ext/object/duplicable'
 
@@ -21,7 +21,7 @@ module ActionDispatch
     #   end
     #   => reverses the value to all keys matching /secret/i
     module FilterParameters
-      @@parameter_filter_for  = {}.extend(Mutex_m)
+      @@parameter_filter_for = ThreadSafe::Cache.new
 
       ENV_MATCH = [/RAW_POST_DATA/, "rack.request.form_vars"] # :nodoc:
       NULL_PARAM_FILTER = ParameterFilter.new # :nodoc:
@@ -65,11 +65,7 @@ module ActionDispatch
       end
 
       def parameter_filter_for(filters)
-        @@parameter_filter_for.synchronize do
-          # Do we *actually* need this cache? Constructing ParameterFilters
-          # doesn't seem too expensive.
-          @@parameter_filter_for[filters] ||= ParameterFilter.new(filters)
-        end
+        @@parameter_filter_for[filters] ||= ParameterFilter.new(filters)
       end
 
       KV_RE   = '[^&;=]+'
diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb
index c11e66d110..eb9d4b24f1 100644
--- a/actionpack/lib/action_dispatch/routing/route_set.rb
+++ b/actionpack/lib/action_dispatch/routing/route_set.rb
@@ -1,5 +1,6 @@
 require 'journey'
 require 'forwardable'
+require 'thread_safe'
 require 'active_support/core_ext/object/to_query'
 require 'active_support/core_ext/hash/slice'
 require 'active_support/core_ext/module/remove_method'
@@ -20,7 +21,7 @@ module ActionDispatch
         def initialize(options={})
           @defaults = options[:defaults]
           @glob_param = options.delete(:glob)
-          @controllers = {}
+          @controller_class_names = ThreadSafe::Cache.new
         end
 
         def call(env)
@@ -68,13 +69,8 @@ module ActionDispatch
       private
 
         def controller_reference(controller_param)
-          controller_name = "#{controller_param.camelize}Controller"
-
-          unless controller = @controllers[controller_param]
-            controller = @controllers[controller_param] =
-              ActiveSupport::Dependencies.reference(controller_name)
-          end
-          controller.get(controller_name)
+          const_name = @controller_class_names[controller_param] ||= "#{controller_param.camelize}Controller"
+          ActiveSupport::Dependencies.constantize(const_name)
         end
 
         def dispatch(controller, action, env)
diff --git a/actionpack/lib/action_view/digestor.rb b/actionpack/lib/action_view/digestor.rb
index 1c6eaf36f7..8bc69b9246 100644
--- a/actionpack/lib/action_view/digestor.rb
+++ b/actionpack/lib/action_view/digestor.rb
@@ -1,4 +1,4 @@
-require 'mutex_m'
+require 'thread_safe'
 
 module ActionView
   class Digestor
@@ -21,23 +21,12 @@ module ActionView
     /x
 
     cattr_reader(:cache)
-    @@cache = Hash.new.extend Mutex_m
+    @@cache = ThreadSafe::Cache.new
 
     def self.digest(name, format, finder, options = {})
-      cache.synchronize do
-        unsafe_digest name, format, finder, options
-      end
-    end
-
-    ###
-    # This method is NOT thread safe.  DO NOT CALL IT DIRECTLY, instead call
-    # Digestor.digest
-    def self.unsafe_digest(name, format, finder, options = {}) # :nodoc:
-      key = "#{name}.#{format}"
-
-      cache.fetch(key) do
+      @@cache["#{name}.#{format}"] ||= begin
         klass = options[:partial] || name.include?("/_") ? PartialDigestor : Digestor
-        cache[key] = klass.new(name, format, finder).digest
+        klass.new(name, format, finder).digest
       end
     end
 
@@ -93,7 +82,7 @@ module ActionView
 
       def dependency_digest
         dependencies.collect do |template_name|
-          Digestor.unsafe_digest(template_name, format, finder, partial: true)
+          Digestor.digest(template_name, format, finder, partial: true)
         end.join("-")
       end
 
diff --git a/actionpack/lib/action_view/lookup_context.rb b/actionpack/lib/action_view/lookup_context.rb
index 76f4dea7b8..4e4816d983 100644
--- a/actionpack/lib/action_view/lookup_context.rb
+++ b/actionpack/lib/action_view/lookup_context.rb
@@ -1,3 +1,4 @@
+require 'thread_safe'
 require 'active_support/core_ext/module/remove_method'
 
 module ActionView
@@ -51,7 +52,7 @@ module ActionView
       alias :object_hash :hash
 
       attr_reader :hash
-      @details_keys = Hash.new
+      @details_keys = ThreadSafe::Cache.new
 
       def self.get(details)
         @details_keys[details] ||= new
diff --git a/actionpack/lib/action_view/renderer/partial_renderer.rb b/actionpack/lib/action_view/renderer/partial_renderer.rb
index 8fb9b6ff18..37f93a13fc 100644
--- a/actionpack/lib/action_view/renderer/partial_renderer.rb
+++ b/actionpack/lib/action_view/renderer/partial_renderer.rb
@@ -1,3 +1,5 @@
+require 'thread_safe'
+
 module ActionView
   # = Action View Partials
   #
@@ -247,7 +249,9 @@ module ActionView
   #     <%- end -%>
   #   <% end %>
   class PartialRenderer < AbstractRenderer
-    PREFIXED_PARTIAL_NAMES = Hash.new { |h,k| h[k] = {} }
+    PREFIXED_PARTIAL_NAMES = ThreadSafe::Cache.new do |h, k|
+      h[k] = ThreadSafe::Cache.new
+    end
 
     def initialize(*)
       super
diff --git a/actionpack/lib/action_view/template/resolver.rb b/actionpack/lib/action_view/template/resolver.rb
index fc77c1485d..8b23029bbc 100644
--- a/actionpack/lib/action_view/template/resolver.rb
+++ b/actionpack/lib/action_view/template/resolver.rb
@@ -3,7 +3,7 @@ require "active_support/core_ext/class"
 require "active_support/core_ext/class/attribute_accessors"
 require "action_view/template"
 require "thread"
-require "mutex_m"
+require "thread_safe"
 
 module ActionView
   # = Action View Resolver
@@ -35,52 +35,51 @@ module ActionView
 
     # Threadsafe template cache
     class Cache #:nodoc:
-      class CacheEntry
-        include Mutex_m
-
-        attr_accessor :templates
+      class SmallCache < ThreadSafe::Cache
+        def initialize(options = {})
+          super(options.merge(:initial_capacity => 2))
+        end
       end
 
+      # preallocate all the default blocks for performance/memory consumption reasons
+      PARTIAL_BLOCK = lambda {|cache, partial| cache[partial] = SmallCache.new}
+      PREFIX_BLOCK  = lambda {|cache, prefix|  cache[prefix]  = SmallCache.new(&PARTIAL_BLOCK)}
+      NAME_BLOCK    = lambda {|cache, name|    cache[name]    = SmallCache.new(&PREFIX_BLOCK)}
+      KEY_BLOCK     = lambda {|cache, key|     cache[key]     = SmallCache.new(&NAME_BLOCK)}
+
+      # usually a majority of template look ups return nothing, use this canonical preallocated array to safe memory
+      NO_TEMPLATES = [].freeze
+
       def initialize
-        @data = Hash.new { |h1,k1| h1[k1] = Hash.new { |h2,k2|
-                  h2[k2] = Hash.new { |h3,k3| h3[k3] = Hash.new { |h4,k4| h4[k4] = {} } } } }
-        @mutex = Mutex.new
+        @data = SmallCache.new(&KEY_BLOCK)
       end
 
       # Cache the templates returned by the block
       def cache(key, name, prefix, partial, locals)
-        cache_entry = nil
-
-        # first obtain a lock on the main data structure to create the cache entry
-        @mutex.synchronize do
-          cache_entry = @data[key][name][prefix][partial][locals] ||= CacheEntry.new
-        end
-
-        # then to avoid a long lasting global lock, obtain a more granular lock
-        # on the CacheEntry itself
-        cache_entry.synchronize do
-          if Resolver.caching?
-            cache_entry.templates ||= yield
+        if Resolver.caching?
+          @data[key][name][prefix][partial][locals] ||= canonical_no_templates(yield)
+        else
+          fresh_templates  = yield
+          cached_templates = @data[key][name][prefix][partial][locals]
+
+          if templates_have_changed?(cached_templates, fresh_templates)
+            @data[key][name][prefix][partial][locals] = canonical_no_templates(fresh_templates)
           else
-            fresh_templates = yield
-
-            if templates_have_changed?(cache_entry.templates, fresh_templates)
-              cache_entry.templates = fresh_templates
-            else
-              cache_entry.templates ||= []
-            end
+            cached_templates || NO_TEMPLATES
           end
         end
       end
 
       def clear
-        @mutex.synchronize do
-          @data.clear
-        end
+        @data.clear
       end
 
       private
 
+      def canonical_no_templates(templates)
+        templates.empty? ? NO_TEMPLATES : templates
+      end
+
       def templates_have_changed?(cached_templates, fresh_templates)
         # if either the old or new template list is empty, we don't need to (and can't)
         # compare modification times, and instead just check whether the lists are different