From fe698bad34dbb11679c6951132bb46375b4ad8af Mon Sep 17 00:00:00 2001 From: "Ryan T. Hosford" Date: Sat, 13 Feb 2016 03:28:05 -0600 Subject: adds tests for Digestor#nested_dependencies --- actionview/test/fixtures/digestor/messages/peek.html.erb | 2 ++ actionview/test/template/digestor_test.rb | 10 ++++++++++ 2 files changed, 12 insertions(+) create mode 100644 actionview/test/fixtures/digestor/messages/peek.html.erb (limited to 'actionview') diff --git a/actionview/test/fixtures/digestor/messages/peek.html.erb b/actionview/test/fixtures/digestor/messages/peek.html.erb new file mode 100644 index 0000000000..84885ab0bc --- /dev/null +++ b/actionview/test/fixtures/digestor/messages/peek.html.erb @@ -0,0 +1,2 @@ +<%# Template Dependency: messages/message %> +<%= render "comments/comments" %> diff --git a/actionview/test/template/digestor_test.rb b/actionview/test/template/digestor_test.rb index bfab97cf1e..e50caac6d8 100644 --- a/actionview/test/template/digestor_test.rb +++ b/actionview/test/template/digestor_test.rb @@ -130,6 +130,16 @@ class TemplateDigestorTest < ActionView::TestCase end end + def test_getting_of_singly_nested_dependencies + singly_nested_dependencies = ["messages/header", "messages/form", "messages/message", "events/event", "comments/comment"] + assert_equal singly_nested_dependencies, nested_dependencies('messages/edit') + end + + def test_getting_of_doubly_nested_dependencies + doubly_nested = [{"comments/comments"=>["comments/comment"]}, "messages/message"] + assert_equal doubly_nested, nested_dependencies('messages/peek') + end + def test_nested_template_directory assert_digest_difference("messages/show") do change_template("messages/actions/_move") -- cgit v1.2.3 From 13c4cc3b5aea02716b7459c0da641438077f5236 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 18 Feb 2016 07:50:53 -0800 Subject: remove object `hash` cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I don't think caching this method makes any difference on Ruby 2.0: ``` require 'benchmark/ips' class Foo alias :object_hash :hash attr_reader :hash def initialize @hash = object_hash end end class Bar end hash = {} foo = Foo.new bar = Bar.new Benchmark.ips do |x| x.report("foo") { hash[foo] } x.report("bar") { hash[bar] } x.report("foo.hash") { foo.hash } x.report("bar.hash") { bar.hash } end __END__ [aaron@TC ruby (trunk)]$ ruby test.rb Warming up -------------------------------------- foo 118.361k i/100ms bar 118.637k i/100ms Calculating ------------------------------------- foo 7.944M (± 3.1%) i/s - 39.769M bar 7.931M (± 3.4%) i/s - 39.625M [aaron@TC ruby (trunk)]$ ruby test.rb Warming up -------------------------------------- foo 122.180k i/100ms bar 120.492k i/100ms foo.hash 123.397k i/100ms bar.hash 119.312k i/100ms Calculating ------------------------------------- foo 8.002M (± 4.2%) i/s - 39.953M bar 8.037M (± 4.5%) i/s - 40.124M foo.hash 8.819M (± 3.9%) i/s - 44.053M bar.hash 7.856M (± 4.1%) i/s - 39.254M ``` --- actionview/lib/action_view/lookup_context.rb | 6 ------ 1 file changed, 6 deletions(-) (limited to 'actionview') diff --git a/actionview/lib/action_view/lookup_context.rb b/actionview/lib/action_view/lookup_context.rb index 126f289f55..2a1f4378f5 100644 --- a/actionview/lib/action_view/lookup_context.rb +++ b/actionview/lib/action_view/lookup_context.rb @@ -55,9 +55,7 @@ module ActionView class DetailsKey #:nodoc: alias :eql? :equal? - alias :object_hash :hash - attr_reader :hash @details_keys = Concurrent::Map.new def self.get(details) @@ -71,10 +69,6 @@ module ActionView def self.clear @details_keys.clear end - - def initialize - @hash = object_hash - end end # Add caching behavior on top of Details. -- cgit v1.2.3 From 3239ed48d28f3c0baf4445e6c279107e892b7cab Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 18 Feb 2016 08:23:26 -0800 Subject: move digest cache on to the DetailsKey object This moves digest calculation cache on to the details key object. Before, the digest cache was a class level ivar, and one of the keys was the hash value of the details key object: https://github.com/rails/rails/blob/13c4cc3b5aea02716b7459c0da641438077f5236/actionview/lib/action_view/digestor.rb#L28 An object's hash value is not unique, so it's possible for this cache key to produce colliding keys with no resolution. This commit move cache on to the details key object itself, so we know that the digests are always unique per details key object. --- actionview/lib/action_view/digestor.rb | 16 +++++++--------- actionview/lib/action_view/lookup_context.rb | 16 ++++++++++++++++ actionview/test/template/digestor_test.rb | 5 ++--- 3 files changed, 25 insertions(+), 12 deletions(-) (limited to 'actionview') diff --git a/actionview/lib/action_view/digestor.rb b/actionview/lib/action_view/digestor.rb index 657026fa14..f3c5d6c8df 100644 --- a/actionview/lib/action_view/digestor.rb +++ b/actionview/lib/action_view/digestor.rb @@ -4,13 +4,11 @@ require 'monitor' module ActionView class Digestor - cattr_reader(:cache) - @@cache = Concurrent::Map.new @@digest_monitor = Monitor.new class PerRequestDigestCacheExpiry < Struct.new(:app) # :nodoc: def call(env) - ActionView::Digestor.cache.clear + ActionView::LookupContext::DetailsKey.clear app.call(env) end end @@ -25,12 +23,12 @@ module ActionView def digest(name:, finder:, **options) options.assert_valid_keys(:dependencies, :partial) - cache_key = ([ name, finder.details_key.hash ].compact + Array.wrap(options[:dependencies])).join('.') + cache_key = ([ name ].compact + Array.wrap(options[:dependencies])).join('.') # this is a correctly done double-checked locking idiom # (Concurrent::Map's lookups have volatile semantics) - @@cache[cache_key] || @@digest_monitor.synchronize do - @@cache.fetch(cache_key) do # re-check under lock + finder.digest_cache[cache_key] || @@digest_monitor.synchronize do + finder.digest_cache.fetch(cache_key) do # re-check under lock compute_and_store_digest(cache_key, name, finder, options) end end @@ -42,16 +40,16 @@ module ActionView # Prevent re-entry or else recursive templates will blow the stack. # There is no need to worry about other threads seeing the +false+ value, # as they will then have to wait for this thread to let go of the @@digest_monitor lock. - pre_stored = @@cache.put_if_absent(cache_key, false).nil? # put_if_absent returns nil on insertion + pre_stored = finder.digest_cache.put_if_absent(cache_key, false).nil? # put_if_absent returns nil on insertion PartialDigestor else Digestor end - @@cache[cache_key] = stored_digest = klass.new(name, finder, options).digest + finder.digest_cache[cache_key] = stored_digest = klass.new(name, finder, options).digest ensure # something went wrong or ActionView::Resolver.caching? is false, make sure not to corrupt the @@cache - @@cache.delete_pair(cache_key, false) if pre_stored && !stored_digest + finder.digest_cache.delete_pair(cache_key, false) if pre_stored && !stored_digest end end diff --git a/actionview/lib/action_view/lookup_context.rb b/actionview/lib/action_view/lookup_context.rb index 2a1f4378f5..86afedaa2d 100644 --- a/actionview/lib/action_view/lookup_context.rb +++ b/actionview/lib/action_view/lookup_context.rb @@ -69,6 +69,18 @@ module ActionView def self.clear @details_keys.clear end + + def self.empty?; @details_keys.empty?; end + + def self.digest_caches + @details_keys.values.map(&:digest_cache) + end + + attr_reader :digest_cache + + def initialize + @digest_cache = Concurrent::Map.new + end end # Add caching behavior on top of Details. @@ -194,6 +206,10 @@ module ActionView self.view_paths = view_paths end + def digest_cache + details_key.digest_cache + end + def initialize_details(target, details) registered_details.each do |k| target[k] = details[k] || Accessors::DEFAULT_PROCS[k].call diff --git a/actionview/test/template/digestor_test.rb b/actionview/test/template/digestor_test.rb index e50caac6d8..9ff5f7126b 100644 --- a/actionview/test/template/digestor_test.rb +++ b/actionview/test/template/digestor_test.rb @@ -33,7 +33,6 @@ class TemplateDigestorTest < ActionView::TestCase def teardown Dir.chdir @cwd FileUtils.rm_r @tmp_dir - ActionView::Digestor.cache.clear end def test_top_level_change_reflected @@ -301,12 +300,12 @@ class TemplateDigestorTest < ActionView::TestCase def assert_digest_difference(template_name, options = {}) previous_digest = digest(template_name, options) - ActionView::Digestor.cache.clear + finder.digest_cache.clear yield assert_not_equal previous_digest, digest(template_name, options), "digest didn't change" - ActionView::Digestor.cache.clear + finder.digest_cache.clear end def digest(template_name, options = {}) -- cgit v1.2.3