Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Avoid locks in template caching - performance improvement for #6404

This change avoids obtaining a lock for templates that are already compiled by
using a thread local copy of any templates already used by the thread.

There are a few issues with this however:
- It breaks ActionView::Template::Resolver#clear_cache, as there's no good way of
  clearing all the Thread local copies. Within Rails it's only used in one test
  (which could be implemented another way). So the question is possibly if this is
  intented to be part of the public API and if it can be removed.
- If there's any scenario in which Resolvers can be re-created there is a potential
  memory leak as there's no attempt to clear up the thread local storage
- I don't have any performance data to justify the need to avoid locks
  • Loading branch information...
commit 69f8bb1b17873bb04a676f89714d224f397597ed 1 parent d3028e3
@pinetops authored
Showing with 49 additions and 9 deletions.
  1. +49 −9 actionpack/lib/action_view/template/resolver.rb
View
58 actionpack/lib/action_view/template/resolver.rb
@@ -35,12 +35,16 @@ class << self
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] = {} } } } }
- @cached_mutex = Mutex.new
+ @cached_monitor = Monitor.new
end
def clear_cache
- @cached_mutex.synchronize {
+ # This method has been fixed just enough to get the tests to pass, but
+ # is no longer really useful. Within rails itself it's not used, but is this
+ # intended to be part of the public API?
+ @cached_monitor.synchronize {
@cached.clear
+ Thread.current["per_thread_template_cache-#{self.object_id}"] = nil
}
end
@@ -55,6 +59,24 @@ def find_all(name, prefix=nil, partial=false, details={}, key=nil, locals=[])
delegate :caching?, :to => "self.class"
+ # The per thread cache
+ def per_thread_cache
+ # First check the thread local cache, and avoid the synchronization lock
+ #
+ # There is no clean up of template caches in the case that the resolver is
+ # replaced - and this might be challenging to implement. Is this likely to
+ # occur?
+ if cache = Thread.current["per_thread_template_cache-#{self.object_id}"]
+ return cache
+ end
+
+ @cached_monitor.synchronize {
+ Thread.current["per_thread_template_cache-#{self.object_id}"] =
+ Hash.new { |h1,k1| h1[k1] = Hash.new { |h2,k2|
+ h2[k2] = Hash.new { |h3,k3| h3[k3] = Hash.new { |h4,k4| h4[k4] = {} } } } }
+ }
+ end
+
# This is what child classes implement. No defaults are needed
# because Resolver guarantees that the arguments are present and
# normalized.
@@ -75,24 +97,42 @@ def cached(key, path_info, details, locals) #:nodoc:
name, prefix, partial = path_info
locals = locals.map { |x| x.to_s }.sort!
- @cached_mutex.synchronize {
- if key && caching?
- @cached[key][name][prefix][partial][locals] ||= decorate(yield, path_info, details, locals)
+ if caching?
+ if key
+ # Access to the cache needs to be synchronized, as Hash accesses aren't Threadsafe
+ # in all Rubys (for example JRuby).
+ #
+ # Cache both in the global cache and additionally in a Thread local cache. This means
+ # that we can avoid synchronizing in the most common situations. This relies on the fact
+ # that one a value is cached it is never replaced
+ if cache = per_thread_cache[key][name][prefix][partial][locals]
+ return cache
+ end
+
+ @cached_monitor.synchronize {
+ per_thread_cache[key][name][prefix][partial][locals] =
+ @cached[key][name][prefix][partial][locals] ||= decorate(yield, path_info, details, locals)
+ }
else
+ decorate(yield, path_info, details, locals)
+ end
+ else
+ # Caching is unlikely to be off in performance sensitive situations so there is
+ # no issue locking in the general case
+ @cached_monitor.synchronize {
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
+ per_thread_cache[key][name][prefix][partial][locals] = scope[locals] = fresh
else
cache
end
- end
- }
+ }
+ end
end
# Ensures all the resolver information is set in the template.
Please sign in to comment.
Something went wrong with that request. Please try again.