Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Make the Resolver template cache threadsafe #6428

Merged
merged 3 commits into from

3 participants

@pinetops

Re-opening #6425 for further discussion as requested by @josevalim in #6427

This is a fix for #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 includes three commits, the first puts a global lock around the whole process. The second improves this to add a more granular lock. Finally, some readability improvements for the caching logic.

pinetops added some commits
@pinetops pinetops 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.
2a12187
@pinetops pinetops 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.
63f3393
@pinetops pinetops Improve the readability of the Resolver change detection code 67bf728
@tenderlove tenderlove was assigned
@wycats wycats merged commit a010fc1 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 21, 2012
  1. @pinetops

    Make the Resolver template cache threadsafe - closes #6404

    pinetops authored
    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.
  2. @pinetops

    More granular locking of the Resolver template cache

    pinetops authored
    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.
Commits on May 22, 2012
  1. @pinetops
This page is out of date. Refresh to see the latest.
View
95 actionpack/lib/action_view/template/resolver.rb
@@ -2,6 +2,7 @@
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,72 @@ def initialize(name, prefix, partial, virtual)
end
end
+ # 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] = {} } } } }
+ @mutex = Mutex.new
+ 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
+ else
+ fresh_templates = yield
+
+ if templates_have_changed?(cache_entry.templates, fresh_templates)
+ cache_entry.templates = fresh_templates
+ else
+ cache_entry.templates ||= []
+ end
+ end
+ end
+ end
+
+ def clear
+ @mutex.synchronize do
+ @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
self.caching = true
@@ -32,12 +99,11 @@ class << self
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 +131,18 @@ def build_path(name, prefix, partial)
# 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
View
8 actionpack/test/template/lookup_context_test.rb
@@ -169,7 +169,7 @@ def teardown
assert_not_equal template, old_template
end
-
+
test "responds to #prefixes" do
assert_equal [], @lookup_context.prefixes
@lookup_context.prefixes = ["foo"]
@@ -180,7 +180,7 @@ def teardown
class LookupContextWithFalseCaching < ActiveSupport::TestCase
def setup
@resolver = ActionView::FixtureResolver.new("test/_foo.erb" => ["Foo", Time.utc(2000)])
- @resolver.stubs(:caching?).returns(false)
+ ActionView::Resolver.stubs(:caching?).returns(false)
@lookup_context = ActionView::LookupContext.new(@resolver, {})
end
@@ -247,6 +247,6 @@ def setup
@lookup_context.view_paths.find("foo", "parent", true, details)
end
assert_match %r{Missing partial parent/foo with .* Searched in:\n \* "/Path/to/views"\n}, e.message
- end
-
+ end
+
end
Something went wrong with that request. Please try again.