Proposal for caching improvements

[vincentwoo]

Regarding the issues brought up in #259...

Motivation

We'd like to make caching in JBuilder useful. Currently, caching does not offer significant performance benefits for pretty much any app. This is largely due to the serialization and over-the-wire costs JBuilder pays when storing and fetching cached content, because the current serialization scheme doesnt allow for raw partial content. Below is a proposal for enabling much faster caching while still retaining MultiJson as the serialization backend:

Proposal

1. When json.cache! cache_key, &block is encountered in a template, Jbuilder emits a placeholder UUID key/value pair into the document (like "jbuilder-XXXX": null) and moves on. It stores the (template digested) cache keys in a list of keys to be looked up, along with their corresponding UUID.

2. After the template is rendered, the half-baked string is held in memory while JBuilder sends a fetch_multi for each of the keys in the set.

3. For each key that is found in the cache, the corresponding UUID is blindly replaced with the value taken from the cache. In this case, a simple gsub for "jbuilder-XXXX": null, replacing it with whatever the cache has, like: "color": "red", "count": 2

4. For each key that is NOT found, execute the block with a fresh json object. Once completed, serialize that json object to a string, and strip off the leading and trailing braces. Store that resulting string under the cache key and perform the substitution operation described in (3). The brace stripping must happen because the json.cache! call does not by default happen under a named key, and can be embedded in the a flow of a template, e.g.

   json.key1 'value1'
   json.cache! do
     json.key2 'value2'
   end

   produces { "key1": "value1", "key2": "value2"}

   Rendering json.key2 'value2' in isolation produces {"key2": "value2"}, which we then strip of braces, which renders it fit to be inserted into the flow of other JSON objects.

Cons

• This is a giant fucking hack.

• Currently, the following case works fine in that only one entry is emitted:

  json.key 'initial value'
  json.cache! do
    json.key 'new value'
  end

  But under the proposal described, JBuilder will double-emit entries for key. By and large this will probably not matter - there's no real reason to do this, and most JSON parsers assume the latest definition takes precedence anyhow.

• Stuff starts to get tricky with nested partials/caches. It should all work fine in theory, there's just going to be a bookkeeping overhead.

...

Thoughts? @rwz, would love to know what you or @dhh think. If you're down, I can probably either build this or browbeat someone into doing it.

[dhh]

This seems like a sound strategy to me. I don't think there's anything hacky about a double-pass rendering process. The caveat re duplicate keys is similar to the caveats we have in HTML views with provide/yield not reaching into a cache. So 👍 from me.

On Aug 22, 2015, at 11:12, Vincent Woo notifications@github.com wrote:

Regarding the issues brought up in #259...

Motivation

We'd like to make caching in JBuilder useful. Currently, caching does not offer significant performance benefits for pretty much any app. This is largely due to the serialization and over-the-wire costs JBuilder pays when storing and fetching cached content, because the current serialization scheme doesnt allow for raw partial content. Below is a proposal for enabling much faster caching while still retaining MultiJson as the serialization backend:

Proposal

When json.cache! cache_key, &block is encountered in a template, Jbuilder emits a placeholder UUID key/value pair into the document (like "jbuilder-XXXX": null) and moves on. It stores the (template digested) cache keys in a list of keys to be looked up, along with their corresponding UUID.

After the template is rendered, the half-baked string is held in memory while JBuilder sends a fetch_multi for each of the keys in the set.

For each key that is found in the cache, the corresponding UUID is blindly replaced with the value taken from the cache. In this case, a simple gsub for "jbuilder-XXXX": null, replacing it with whatever the cache has, like: "color": "red", "count": 2

For each key that is NOT found, execute the block with a fresh json object. Once completed, serialize that json object to a string, and strip off the leading and trailing braces. Store that resulting string under the cache key and perform the substitution operation described in (3). The brace stripping must happen because the json.cache! call does not by default happen under a named key, and can be embedded in the a flow of a template, e.g.

json.key1 'value1'
json.cache! do
json.key2 'value2'
end
produces { "key1": "value1", "key2": "value2"}

Rendering json.key2 'value2' in isolation produces {"key2": "value2"}, which we then strip of braces, which renders it fit to be inserted into the flow of other JSON objects.

Cons

This is a giant fucking hack.
Currently, the following case works fine in that only one entry is emitted:

json.key 'initial value'
json.cache! do
json.key 'new value'
end
But under the described proposal, JBuilder could double-emit entries for key. By and large this will probably not matter - there's no real reason to do this, and most JSON parsers assume the latest definition is takes precedence anyhow.

Stuff starts to get tricky with nested partials/caches. It should all work fine in theory, there's just going to be a bookkeeping overhead.

...

Thoughts? @rwz, would love to know what you or @dhh think. If you're down, I can probably either build this or browbeat someone into doing it.

—
Reply to this email directly or view it on GitHub.

[rwz]

I'm skeptical about this approach for various reasons. First, seems it's going to increase code complexity quite a lot to make it work with all possible cases, like calling cache in the middle of rendering an array and such. Also, if you're delaying the execution of cached blocks until after the parent template is fully rendered and jsonifyed, you're losing some context and I'm not sure it's entirely safe and not gonna break some of the exiting workflows.

But even given we're willing to take the risk, it'd be nice to see some benchmarks proving it's actually going to give a significant performance boost. Are you willing to take a stab at this and prepare a branch implementing this change so we can compare the performance against master on some existing apps that rely on cache heavily?

[vincentwoo]

I could try to prep a sample app that uses JBuilder caching heavily. As far as I know, no serious app actually uses JBuilder caching because of the performance impact.

Question - is it possible for cache blocks to introspect the state of the json? If so, that's going to be very annoying for compatibility reasons. Otherwise, I don't think losing context will matter, blocks should still work.

[tortuetorche]

This is largely due to the serialization and over-the-wire costs JBuilder pays when storing and fetching cached content, because the current serialization scheme doesnt allow for raw partial content.

@vincentwoo Maybe it's possible to change the current behavior to store JBuilder data without serialization? Via the :raw option, like explained in the Rails Guides, it should work with memcache and redis cache stores.

[vincentwoo]

You can't. The raw option is for something else (storing pure numbers/strings). Because JBuilder currently works by building a very large in-memory object and only serializing at the end, partial content cannot be stored as raw strings. This proposal aims to allow storing raw partial content.

[chirag7jain]

Why can't we just create a hash and store it into our cache store and just feed it to jbuilder.

I am a newbie to rails & jbuilder.. Just ignore it if I am speaking out of context

[vincentwoo]

Check out the related issue for an in depth explanation of why you can't: #259

[dirk]

I will be implementing this proposal in Everlane's fork.

[chirag7jain]

I can collaborate on this if you need help

On Sat, Oct 3, 2015, 06:49 Dirk Gadsden notifications@github.com wrote:

I will be implementing this proposal in Everlane's fork.

—
Reply to this email directly or view it on GitHub.

[dirk]

@VincentWo, @rwz, @dhh: This has been implemented in PR #298.

[vincentwoo]

I think we'd want to see side-by-sides and measureables. Can you produce a testing repo? Maybe use a stripped-down and scrubbed copy of Everlane's jbuilder structure?

https://github.com/evanphx/benchmark-ips is the standard for Rails perf PRs

[dirk]

@vincentwoo: I'll put a side-by-side together today.

[dirk]

Here are the results from benchmarking with this script:

$ ruby -v
ruby 2.2.1p85 (2015-02-26 revision 49769) [x86_64-darwin14]

$ git checkout master
$ ruby benchmark.rb
Calculating -------------------------------------
               basic     3.000  i/100ms
-------------------------------------------------
               basic     46.538  (± 4.3%) i/s -    234.000

$ git checkout dirk/caching-improvements
$ ruby benchmark.rb
Calculating -------------------------------------
               basic     9.000  i/100ms
-------------------------------------------------
               basic    112.021  (± 5.4%) i/s -    567.000