From 7714c7784cb5ca76228ffc97cb90009fa889444a Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Tue, 15 Nov 2016 16:46:53 +0100 Subject: [PATCH 1/8] Add cache_root! for faster root caching Optimization trick extracted from Basecamp --- lib/jbuilder.rb | 3 ++- lib/jbuilder/jbuilder_template.rb | 24 ++++++++++++++++++++++++ test/jbuilder_template_test.rb | 29 +++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 1 deletion(-) diff --git a/lib/jbuilder.rb b/lib/jbuilder.rb index c330532..4a37219 100644 --- a/lib/jbuilder.rb +++ b/lib/jbuilder.rb @@ -14,6 +14,7 @@ def initialize(options = {}) @key_formatter = options.fetch(:key_formatter){ @@key_formatter ? @@key_formatter.clone : nil} @ignore_nil = options.fetch(:ignore_nil, @@ignore_nil) + @cached_root = nil yield self if ::Kernel.block_given? end @@ -247,7 +248,7 @@ def merge!(hash_or_array) # Encodes the current builder as JSON. def target! - ::MultiJson.dump(@attributes) + @cached_root || ::MultiJson.dump(@attributes) end private diff --git a/lib/jbuilder/jbuilder_template.rb b/lib/jbuilder/jbuilder_template.rb index 882c030..c4a7143 100644 --- a/lib/jbuilder/jbuilder_template.rb +++ b/lib/jbuilder/jbuilder_template.rb @@ -42,6 +42,30 @@ def cache!(key=nil, options={}) end end + # Caches the json structure at the root using a string rather than the hash structure. This is considerably + # faster, but the drawback is that it only works, as the name hints, at the root. So you cannot + # use this approach to cache deeper inside the hierarchy, like in partials or such. Continue to use #cache! there. + # + # Example: + # + # json.cache_root! @person do + # json.extract! @person, :name, :age + # end + # + # # json.extra 'This will not work either, the root must be exclusive' + def cache_root!(key=nil, options={}) + if @context.controller.perform_caching + raise "cache_root! can't be used after JSON structures have been defined" if @attributes.present? + + @cached_root = _cache_fragment_for([ :root, key ], options) do + yield + target! + end + else + yield + end + end + # Conditionally caches the json depending in the condition given as first parameter. Has the same # signature as the `cache` helper method in `ActionView::Helpers::CacheHelper` and so can be used in # the same way. diff --git a/test/jbuilder_template_test.rb b/test/jbuilder_template_test.rb index e302881..d532128 100644 --- a/test/jbuilder_template_test.rb +++ b/test/jbuilder_template_test.rb @@ -386,6 +386,35 @@ def assert_collection_rendered(result, context = nil) JBUILDER end + test "caching root structure" do + undef_context_methods :fragment_name_with_digest, :cache_fragment_name + + cache_miss_result = jbuild <<-JBUILDER + json.cache_root! "cachekey" do + json.name "Miss" + end + JBUILDER + + cache_hit_result = jbuild <<-JBUILDER + json.cache_root! "cachekey" do + json.name "Hit" + end + JBUILDER + + assert_equal cache_miss_result, cache_hit_result + end + + test "failing to cache root after attributes have been defined" do + assert_raises "cache_root! can't be used after JSON structures have been defined" do + jbuild <<-JBUILDER + json.name "Kaboom" + json.cache_root! "cachekey" do + json.name "Miss" + end + JBUILDER + end + end + test "does not perform caching when controller.perform_caching is false" do controller.perform_caching = false From 0be2d50f706d896b3403976d320f6f335c9592ab Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Tue, 15 Nov 2016 16:46:58 +0100 Subject: [PATCH 2/8] Styling --- lib/jbuilder.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/jbuilder.rb b/lib/jbuilder.rb index 4a37219..2ae0cb0 100644 --- a/lib/jbuilder.rb +++ b/lib/jbuilder.rb @@ -11,9 +11,10 @@ class Jbuilder def initialize(options = {}) @attributes = {} + + @key_formatter = options.fetch(:key_formatter) { @@key_formatter ? @@key_formatter.clone : nil } + @ignore_nil = options.fetch(:ignore_nil, @@ignore_nil) - @key_formatter = options.fetch(:key_formatter){ @@key_formatter ? @@key_formatter.clone : nil} - @ignore_nil = options.fetch(:ignore_nil, @@ignore_nil) @cached_root = nil yield self if ::Kernel.block_given? From 1a2e291f7f9964b7374d86accf09ca309be5d101 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Mon, 28 Nov 2016 13:26:34 +0100 Subject: [PATCH 3/8] Caching is a JbuilderTemplate concern, lift up the cached root look-up --- lib/jbuilder.rb | 2 +- lib/jbuilder/jbuilder_template.rb | 9 +++++---- test/jbuilder_template_test.rb | 2 +- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/jbuilder.rb b/lib/jbuilder.rb index 2ae0cb0..cedd36a 100644 --- a/lib/jbuilder.rb +++ b/lib/jbuilder.rb @@ -249,7 +249,7 @@ def merge!(hash_or_array) # Encodes the current builder as JSON. def target! - @cached_root || ::MultiJson.dump(@attributes) + ::MultiJson.dump(@attributes) end private diff --git a/lib/jbuilder/jbuilder_template.rb b/lib/jbuilder/jbuilder_template.rb index c4a7143..8dda9c5 100644 --- a/lib/jbuilder/jbuilder_template.rb +++ b/lib/jbuilder/jbuilder_template.rb @@ -57,10 +57,7 @@ def cache_root!(key=nil, options={}) if @context.controller.perform_caching raise "cache_root! can't be used after JSON structures have been defined" if @attributes.present? - @cached_root = _cache_fragment_for([ :root, key ], options) do - yield - target! - end + @cached_root = _cache_fragment_for([ :root, key ], options) { yield; target! } else yield end @@ -99,6 +96,10 @@ def set!(name, object = BLANK, *args) end end + def target! + @cached_root || super + end + private def _render_partial_with_options(options) diff --git a/test/jbuilder_template_test.rb b/test/jbuilder_template_test.rb index d532128..b62277d 100644 --- a/test/jbuilder_template_test.rb +++ b/test/jbuilder_template_test.rb @@ -405,7 +405,7 @@ def assert_collection_rendered(result, context = nil) end test "failing to cache root after attributes have been defined" do - assert_raises "cache_root! can't be used after JSON structures have been defined" do + assert_raises ActionView::Template::Error, "cache_root! can't be used after JSON structures have been defined" do jbuild <<-JBUILDER json.name "Kaboom" json.cache_root! "cachekey" do From 92cdafc8a30dd5db6b3c84b9f6fb9af8c9e2d764 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Mon, 28 Nov 2016 13:27:09 +0100 Subject: [PATCH 4/8] Cluster caching concerns together --- lib/jbuilder/jbuilder_template.rb | 40 +++++++++++++++---------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/lib/jbuilder/jbuilder_template.rb b/lib/jbuilder/jbuilder_template.rb index 8dda9c5..08b0ecf 100644 --- a/lib/jbuilder/jbuilder_template.rb +++ b/lib/jbuilder/jbuilder_template.rb @@ -22,6 +22,26 @@ def partial!(*args) end end + def array!(collection = [], *args) + options = args.first + + if args.one? && _partial_options?(options) + partial! options.merge(collection: collection) + else + super + end + end + + def set!(name, object = BLANK, *args) + options = args.first + + if args.one? && _partial_options?(options) + _set_inline_partial name, object, options + else + super + end + end + # Caches the json constructed within the block passed. Has the same signature as the `cache` helper # method in `ActionView::Helpers::CacheHelper` and so can be used in the same way. # @@ -76,26 +96,6 @@ def cache_if!(condition, *args) condition ? cache!(*args, &::Proc.new) : yield end - def array!(collection = [], *args) - options = args.first - - if args.one? && _partial_options?(options) - partial! options.merge(collection: collection) - else - super - end - end - - def set!(name, object = BLANK, *args) - options = args.first - - if args.one? && _partial_options?(options) - _set_inline_partial name, object, options - else - super - end - end - def target! @cached_root || super end From 471181fd7c14b6c5245042aa7caa4296f370a4f1 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Mon, 28 Nov 2016 13:29:15 +0100 Subject: [PATCH 5/8] No longer needed --- lib/jbuilder.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/jbuilder.rb b/lib/jbuilder.rb index cedd36a..6232c44 100644 --- a/lib/jbuilder.rb +++ b/lib/jbuilder.rb @@ -15,8 +15,6 @@ def initialize(options = {}) @key_formatter = options.fetch(:key_formatter) { @@key_formatter ? @@key_formatter.clone : nil } @ignore_nil = options.fetch(:ignore_nil, @@ignore_nil) - @cached_root = nil - yield self if ::Kernel.block_given? end From 9877822e58336378746b2df1650c9c4586625eab Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Mon, 28 Nov 2016 13:32:40 +0100 Subject: [PATCH 6/8] Let's do style changes in a separate PR --- lib/jbuilder.rb | 7 +++--- lib/jbuilder/jbuilder_template.rb | 40 +++++++++++++++---------------- 2 files changed, 23 insertions(+), 24 deletions(-) diff --git a/lib/jbuilder.rb b/lib/jbuilder.rb index 6232c44..78ff98f 100644 --- a/lib/jbuilder.rb +++ b/lib/jbuilder.rb @@ -10,10 +10,9 @@ class Jbuilder @@ignore_nil = false def initialize(options = {}) - @attributes = {} - - @key_formatter = options.fetch(:key_formatter) { @@key_formatter ? @@key_formatter.clone : nil } - @ignore_nil = options.fetch(:ignore_nil, @@ignore_nil) + @attributes = {} + @key_formatter = options.fetch(:key_formatter){ @@key_formatter ? @@key_formatter.clone : nil} + @ignore_nil = options.fetch(:ignore_nil, @@ignore_nil) yield self if ::Kernel.block_given? end diff --git a/lib/jbuilder/jbuilder_template.rb b/lib/jbuilder/jbuilder_template.rb index 08b0ecf..feaee62 100644 --- a/lib/jbuilder/jbuilder_template.rb +++ b/lib/jbuilder/jbuilder_template.rb @@ -22,26 +22,6 @@ def partial!(*args) end end - def array!(collection = [], *args) - options = args.first - - if args.one? && _partial_options?(options) - partial! options.merge(collection: collection) - else - super - end - end - - def set!(name, object = BLANK, *args) - options = args.first - - if args.one? && _partial_options?(options) - _set_inline_partial name, object, options - else - super - end - end - # Caches the json constructed within the block passed. Has the same signature as the `cache` helper # method in `ActionView::Helpers::CacheHelper` and so can be used in the same way. # @@ -100,6 +80,26 @@ def target! @cached_root || super end + def array!(collection = [], *args) + options = args.first + + if args.one? && _partial_options?(options) + partial! options.merge(collection: collection) + else + super + end + end + + def set!(name, object = BLANK, *args) + options = args.first + + if args.one? && _partial_options?(options) + _set_inline_partial name, object, options + else + super + end + end + private def _render_partial_with_options(options) From d59fff7bd37d9936223797cc15af54d84d1baf2e Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Mon, 28 Nov 2016 13:33:25 +0100 Subject: [PATCH 7/8] There was a CR here before --- lib/jbuilder.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/jbuilder.rb b/lib/jbuilder.rb index 78ff98f..0925b19 100644 --- a/lib/jbuilder.rb +++ b/lib/jbuilder.rb @@ -11,6 +11,7 @@ class Jbuilder def initialize(options = {}) @attributes = {} + @key_formatter = options.fetch(:key_formatter){ @@key_formatter ? @@key_formatter.clone : nil} @ignore_nil = options.fetch(:ignore_nil, @@ignore_nil) From bc698f963717378cfcc59c9455aa02e31aa8a037 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Mon, 28 Nov 2016 13:33:50 +0100 Subject: [PATCH 8/8] Fucking spacing --- lib/jbuilder.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/jbuilder.rb b/lib/jbuilder.rb index 0925b19..c330532 100644 --- a/lib/jbuilder.rb +++ b/lib/jbuilder.rb @@ -10,7 +10,7 @@ class Jbuilder @@ignore_nil = false def initialize(options = {}) - @attributes = {} + @attributes = {} @key_formatter = options.fetch(:key_formatter){ @@key_formatter ? @@key_formatter.clone : nil} @ignore_nil = options.fetch(:ignore_nil, @@ignore_nil)