From 2a121870a6aba2b40d65e75d8fa4f7b959aac2c9 Mon Sep 17 00:00:00 2001 From: Tom Clarke Date: Sun, 20 May 2012 01:03:34 -0400 Subject: Make the Resolver template cache threadsafe - closes #6404 The Template cache in the Resolver can be accessed by multiple threads similtaneously in multi-threaded environments. The cache is implemented using a Hash, which isn't threadsafe in all VMs (notably JRuby). This commit extracts the cache to a new Cache class and adds mutexes to prevent concurrent access. --- actionpack/lib/action_view/template/resolver.rb | 69 ++++++++++++++++++------- 1 file changed, 50 insertions(+), 19 deletions(-) (limited to 'actionpack/lib/action_view') diff --git a/actionpack/lib/action_view/template/resolver.rb b/actionpack/lib/action_view/template/resolver.rb index fa2038f78d..b33aebd3bc 100644 --- a/actionpack/lib/action_view/template/resolver.rb +++ b/actionpack/lib/action_view/template/resolver.rb @@ -2,6 +2,7 @@ require "pathname" require "active_support/core_ext/class" require "active_support/core_ext/class/attribute_accessors" require "action_view/template" +require "thread" module ActionView # = Action View Resolver @@ -24,6 +25,46 @@ module ActionView end end + # Threadsafe template cache + class Cache #:nodoc: + 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 + end + + # Cache the templates returned by the block + def cache(key, name, prefix, partial, locals) + @mutex.synchronize do + if Resolver.caching? + # all templates are cached forever the first time they are accessed + @data[key][name][prefix][partial][locals] ||= yield + else + # templates are still cached, but are only returned if they are + # all still current + fresh = yield + + cache = @data[key][name][prefix][partial][locals] + mtime = cache && cache.map(&:updated_at).max + + newer = !mtime || fresh.empty? || fresh.any? { |t| t.updated_at > mtime } + + if newer + @data[key][name][prefix][partial][locals] = fresh + else + @data[key][name][prefix][partial][locals] + end + end + end + end + + def clear + @mutex.synchronize do + @data.clear + end + end + end + cattr_accessor :caching self.caching = true @@ -32,12 +73,11 @@ module ActionView end def initialize - @cached = Hash.new { |h1,k1| h1[k1] = Hash.new { |h2,k2| - h2[k2] = Hash.new { |h3,k3| h3[k3] = Hash.new { |h4,k4| h4[k4] = {} } } } } + @cache = Cache.new end def clear_cache - @cached.clear + @cache.clear end # Normalizes the arguments and passes it on to find_template. @@ -65,27 +105,18 @@ module ActionView # Handles templates caching. If a key is given and caching is on # always check the cache before hitting the resolver. Otherwise, - # it always hits the resolver but check if the resolver is fresher - # before returning it. + # it always hits the resolver but if the key is present, check if the + # resolver is fresher before returning it. def cached(key, path_info, details, locals) #:nodoc: name, prefix, partial = path_info locals = locals.map { |x| x.to_s }.sort! - if key && caching? - @cached[key][name][prefix][partial][locals] ||= decorate(yield, path_info, details, locals) - else - fresh = decorate(yield, path_info, details, locals) - return fresh unless key - - scope = @cached[key][name][prefix][partial] - cache = scope[locals] - mtime = cache && cache.map(&:updated_at).max - - if !mtime || fresh.empty? || fresh.any? { |t| t.updated_at > mtime } - scope[locals] = fresh - else - cache + if key + @cache.cache(key, name, prefix, partial, locals) do + decorate(yield, path_info, details, locals) end + else + decorate(yield, path_info, details, locals) end end -- cgit v1.2.3 From 63f3393f88a160d5bf87c84d7317a16379587280 Mon Sep 17 00:00:00 2001 From: Tom Clarke Date: Mon, 21 May 2012 09:41:04 -0400 Subject: More granular locking of the Resolver template cache In order to avoid holding a global lock when doing template resolution, instead add individual locks on a per cache entry basis. The global lock is now only used for manipulation of the main cache data structure. --- actionpack/lib/action_view/template/resolver.rb | 28 ++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) (limited to 'actionpack/lib/action_view') diff --git a/actionpack/lib/action_view/template/resolver.rb b/actionpack/lib/action_view/template/resolver.rb index b33aebd3bc..8f87d6da8b 100644 --- a/actionpack/lib/action_view/template/resolver.rb +++ b/actionpack/lib/action_view/template/resolver.rb @@ -27,6 +27,16 @@ module ActionView # Threadsafe template cache class Cache #:nodoc: + class CacheEntry + attr_accessor :templates + + delegate :synchronize, :to => "@mutex" + + def initialize + @mutex = Mutex.new + end + end + 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] = {} } } } } @@ -35,24 +45,32 @@ module ActionView # 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? # all templates are cached forever the first time they are accessed - @data[key][name][prefix][partial][locals] ||= yield + cache_entry.templates ||= yield else # templates are still cached, but are only returned if they are # all still current fresh = yield - cache = @data[key][name][prefix][partial][locals] - mtime = cache && cache.map(&:updated_at).max + mtime = cache_entry.templates && cache_entry.templates.map(&:updated_at).max newer = !mtime || fresh.empty? || fresh.any? { |t| t.updated_at > mtime } if newer - @data[key][name][prefix][partial][locals] = fresh + cache_entry.templates = fresh else - @data[key][name][prefix][partial][locals] + cache_entry.templates end end end -- cgit v1.2.3 From 67bf728a1f53305c935f29e7ff3e23c5ac58eda5 Mon Sep 17 00:00:00 2001 From: Tom Clarke Date: Mon, 21 May 2012 21:31:40 -0400 Subject: Improve the readability of the Resolver change detection code --- actionpack/lib/action_view/template/resolver.rb | 30 ++++++++++++++++--------- 1 file changed, 19 insertions(+), 11 deletions(-) (limited to 'actionpack/lib/action_view') diff --git a/actionpack/lib/action_view/template/resolver.rb b/actionpack/lib/action_view/template/resolver.rb index 8f87d6da8b..ab09dbece4 100644 --- a/actionpack/lib/action_view/template/resolver.rb +++ b/actionpack/lib/action_view/template/resolver.rb @@ -56,21 +56,14 @@ module ActionView # on the CacheEntry itself cache_entry.synchronize do if Resolver.caching? - # all templates are cached forever the first time they are accessed cache_entry.templates ||= yield else - # templates are still cached, but are only returned if they are - # all still current - fresh = yield + fresh_templates = yield - mtime = cache_entry.templates && cache_entry.templates.map(&:updated_at).max - - newer = !mtime || fresh.empty? || fresh.any? { |t| t.updated_at > mtime } - - if newer - cache_entry.templates = fresh + if templates_have_changed?(cache_entry.templates, fresh_templates) + cache_entry.templates = fresh_templates else - cache_entry.templates + cache_entry.templates ||= [] end end end @@ -81,6 +74,21 @@ module ActionView @data.clear end end + + private + + 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 + if cached_templates.blank? || fresh_templates.blank? + return fresh_templates.blank? != cached_templates.blank? + end + + cached_templates_max_updated_at = cached_templates.map(&:updated_at).max + + # if a template has changed, it will be now be newer than all the cached templates + fresh_templates.any? { |t| t.updated_at > cached_templates_max_updated_at } + end end cattr_accessor :caching -- cgit v1.2.3