Skip to content

Commit

Permalink
Use fetch_multi instead of fetch
Browse files Browse the repository at this point in the history
This will improve the performance since we are going to hit the backend
just once to read all keys, as long the cache adapter implements fetch
multi support like dalli.

For example:

    builder.cache! :x do |cache|
      cache.x true
    end

    builder.cache! :y do |cache|
      cache.y true
    end

    builder.cache! :z do |cache|
      cache.z true
    end

This example was hitting the memcached 6 times on cache miss:

  1. read x
  2. write x
  3. read y
  4. write y
  5. read z
  6. write z

And 3 times on cache hit:

  1. read x
  2. read y
  3. read z

After this change, 4 times on cache miss:

  1. read multi x,y,z
  2. write x
  3. write y
  4. write z

And 1 time on cache hit:

  1. read multi x,y,z

Note that in the case of different options, one read multi will be made
per each options, i.e.:

    builder.cache! :x do |cache|
      cache.x true
    end

    builder.cache! :y do |cache|
      cache.y true
    end

    builder.cache! :z, expires_in: 10.minutes do |cache|
      cache.z true
    end

    builder.cache! :w, expires_in: 10.minutes do |cache|
      cache.w true
    end

In the case of cache miss:

  1. read multi x,y
  2. write x
  3. write y
  4. read multi z,w
  5. write z
  5. write w

In the case of cache hit:

  1. read multi x,y
  2. read multi z,w

That's because Rails.cache.fetch_multi signature is limited to use the
same options for all given keys.

And for last, nested cache calls are allowed and will follow recursively
to accomplish the same behavior, i.e.:

  builder.cache! :x do |cache_x|
    cache_x.x true

    cache_x.cache! :y do |cache_y|
      cache_y.y true
    end

    cache_x.cache! :z do |cache_z|
      cache_z.z true
    end
  end

  builder.cache! :w do |cache_w|
    cache_w.w true
  end

In the case of cache miss:

  1. read multi x,w
  2. read multi y,z
  3. write y
  4. write z
  5. write x
  6. write w

In the case of cache hit:

  1. read multi x,w

The same rule of options will be applied, if you have different options,
one hit per options.

Pretty much the same of rails/jbuilder#421.
  • Loading branch information
sobrinho committed Nov 28, 2017
1 parent 8e4d0bf commit 8891ea1
Show file tree
Hide file tree
Showing 6 changed files with 240 additions and 35 deletions.
19 changes: 15 additions & 4 deletions lib/abstract_builder.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
require 'abstract_builder/null_cache'
require 'abstract_builder/lazy_cache'

class AbstractBuilder
@@format_key = nil
Expand All @@ -21,6 +22,7 @@ def initialize
@format_key = @@format_key
@ignore_value = @@ignore_value
@cache_store = @@cache_store
@lazy_cache = LazyCache.new(@@cache_store)
@stack = []
end

Expand All @@ -34,6 +36,7 @@ def ignore_value!(&block)

def cache_store!(cache_store)
@cache_store = cache_store
@lazy_cache = LazyCache.new(cache_store)
end

def set!(key, value)
Expand Down Expand Up @@ -70,14 +73,14 @@ def array!(key, collection, &block)
set! key, values
end

def cache!(key, options = nil, &block)
value = @cache_store.fetch([:abstract_builder, :v1, *key], options) do
def cache!(cache_key, options = {}, &block)
cache_key = _compute_cache_key(cache_key)

@lazy_cache.add(cache_key, options) do
builder = _inherit
block.call(builder)
builder.data!
end

merge! value
end

def data!
Expand All @@ -88,6 +91,10 @@ def data!
data[key] = value unless _ignore_value?(value)
end

@lazy_cache.resolve.each do |value|
data.merge!(value)
end

data
end

Expand Down Expand Up @@ -133,6 +140,10 @@ def _inherit
builder
end

def _compute_cache_key(key)
[:abstract_builder, :v1, *key].join("/".freeze)
end

def _ignore_value?(value)
@ignore_value && @ignore_value.call(value)
end
Expand Down
42 changes: 42 additions & 0 deletions lib/abstract_builder/lazy_cache.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
class AbstractBuilder
class LazyCache
def initialize(driver)
@cache = Hash.new { |h, k| h[k] = {} }
@driver = driver
end

def add(key, options, &block)
cache[options][key] = block
end

def resolve
resolved = []

# Fail-fast if there is no items to be computed.
return resolved if cache.empty?

# We can't add new items during interation, so iterate through a clone
# that will allow us to add new items.
previous = cache.clone
cache.clear

# Keys are grouped by options and because of that, fetch_multi will use
# the same options for the same group of keys.
previous.each do |options, group|
result = driver.fetch_multi(*group.keys, options) do |group_key|
[group[group_key].call, *resolve]
end

# Since the fetch_multi returns { cache_key => value }, we need to
# discard the cache key and merge only the values.
resolved.concat result.values.flatten(1)
end

resolved
end

private

attr_reader :cache, :driver
end
end
12 changes: 12 additions & 0 deletions lib/abstract_builder/null_cache.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,17 @@ class NullCache
def fetch(key, _options = nil, &block)
block.call
end

def fetch_multi(*keys, options, &block)
result = {}

keys.each do |key|
result[key] = fetch(key, options) do
block.call(key)
end
end

result
end
end
end
75 changes: 75 additions & 0 deletions spec/abstract_builder/lazy_cache_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
RSpec.describe AbstractBuilder::LazyCache do
it "resolves all entries at once using fetch multi" do
driver = NaiveCache.new

expect(driver).to receive(:fetch_multi).with("x", "y", "z", {}).and_call_original.exactly(3).times

3.times do
lazy_cache = described_class.new(driver)
lazy_cache.add("x", {}) { { x: true } }
lazy_cache.add("y", {}) { { y: true } }
lazy_cache.add("z", {}) { { z: true } }

expect(lazy_cache.resolve).to eq [
{ x: true },
{ y: true },
{ z: true }
]
end
end

it "resolves all entries at once per options using fetch multi" do
driver = NaiveCache.new

expect(driver).to receive(:fetch_multi).with("x", { option: false }).and_call_original.exactly(3).times
expect(driver).to receive(:fetch_multi).with("y", "z", { option: true }).and_call_original.exactly(3).times

3.times do
lazy_cache = described_class.new(driver)
lazy_cache.add("x", { option: false }) { { x: true } }
lazy_cache.add("y", { option: true }) { { y: true } }
lazy_cache.add("z", { option: true }) { { z: true } }

expect(lazy_cache.resolve).to eq [
{ x: true },
{ y: true },
{ z: true }
]
end
end

it 'resolves nested entries at once using fetch multi' do
driver = NaiveCache.new

expect(driver).to receive(:fetch_multi).with("x", {}).and_call_original.exactly(3).times
expect(driver).to receive(:fetch_multi).with("y", "z", { option: false }).and_call_original.exactly(1).times
expect(driver).to receive(:fetch_multi).with("w", { option: true }).and_call_original.exactly(1).times

3.times do
lazy_cache = described_class.new(driver)

lazy_cache.add("x", {}) do
lazy_cache.add("y", { option: false }) do
{ y: true }
end

lazy_cache.add("z", { option: false }) do
{ z: true }
end

lazy_cache.add("w", { option: true }) do
{ w: true }
end

{ x: true }
end

expect(lazy_cache.resolve).to eq [
{ x: true },
{ y: true },
{ z: true },
{ w: true }
]
end
end
end
113 changes: 83 additions & 30 deletions spec/abstract_builder_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -52,32 +52,38 @@
it "caches using the given cache store" do
subject.cache_store! cache_store

expect(cache_store).to receive(:fetch).with([:abstract_builder, :v1, :cache_key], nil).and_call_original
expect(cache_store).to receive(:fetch_multi).with("abstract_builder/v1/cache_key", {}).and_call_original

subject.cache! :cache_key do |builder|
builder.cache "hit"
end

subject.data!
end

it "caches using the given options" do
subject.cache_store! cache_store

expect(cache_store).to receive(:fetch).with([:abstract_builder, :v1, :cache_key], option: true).and_call_original
expect(cache_store).to receive(:fetch_multi).with("abstract_builder/v1/cache_key", option: true).and_call_original

subject.cache! :cache_key, option: true do |builder|
builder.cache "hit"
end

subject.data!
end

it "inherits the global ignore value by default" do
begin
AbstractBuilder.cache_store! cache_store

expect(cache_store).to receive(:fetch).with([:abstract_builder, :v1, :cache_key], nil).and_call_original
expect(cache_store).to receive(:fetch_multi).with("abstract_builder/v1/cache_key", {}).and_call_original

subject.cache! :cache_key do |builder|
builder.cache "hit"
end

subject.data!
ensure
AbstractBuilder.cache_store! AbstractBuilder::NullCache.new
end
Expand Down Expand Up @@ -220,27 +226,44 @@

context "using cache store" do
it "inherits the parent cache store" do
subject.cache_store! NaiveCache.new
driver = NaiveCache.new

subject.cache! :outside_cache_key do |builder|
builder.outside_cache "hit"
end
# cache miss
builder = described_class.new
builder.cache_store! driver

subject.cache! :outside_cache_key do |builder|
builder.outside_cache "miss"
builder.cache! :outside_cache_key do |builder|
builder.outside_cache "hit"
end

subject.block! :meta do |meta|
builder.block! :meta do |meta|
meta.cache! :inside_cache_key do |builder|
builder.inside_cache "hit"
end
end

expect(builder.data!).to eq(
outside_cache: "hit",
meta: {
inside_cache: "hit"
}
)

# cache hit
builder = described_class.new
builder.cache_store! driver

builder.cache! :outside_cache_key do |builder|
builder.outside_cache "miss"
end

builder.block! :meta do |meta|
meta.cache! :inside_cache_key do |builder|
builder.inside_cache "miss"
end
end

expect(subject.data!).to eq(
expect(builder.data!).to eq(
outside_cache: "hit",
meta: {
inside_cache: "hit"
Expand All @@ -249,33 +272,53 @@
end

it "do not leaks the ignore value to the parent" do
subject.cache_store! AbstractBuilder::NullCache.new

subject.cache! :outside_cache_key do |builder|
builder.outside_cache "hit"
end
null_driver = AbstractBuilder::NullCache.new
naive_driver = NaiveCache.new

subject.cache! :outside_cache_key do |builder|
builder.outside_cache "miss"
end
# cache miss
builder = described_class.new
builder.cache_store! null_driver

subject.block! :meta do |meta|
meta.cache_store! NaiveCache.new
builder.block! :meta do |meta|
meta.cache_store! naive_driver

meta.cache! :inside_cache_key do |builder|
builder.inside_cache "hit"
end
end

builder.cache! :outside_cache_key do |builder|
builder.outside_cache "hit"
end

expect(builder.data!).to eq(
meta: {
inside_cache: "hit"
},
outside_cache: "hit"
)

# cache hit
builder = described_class.new
builder.cache_store! null_driver

builder.block! :meta do |meta|
meta.cache_store! naive_driver

meta.cache! :inside_cache_key do |builder|
builder.inside_cache "miss"
end
end

expect(subject.data!).to eq(
outside_cache: "miss",
builder.cache! :outside_cache_key do |builder|
builder.outside_cache "miss"
end

expect(builder.data!).to eq(
meta: {
inside_cache: "hit"
}
},
outside_cache: "miss"
)
end
end
Expand Down Expand Up @@ -303,17 +346,27 @@

describe "#cache!" do
it "caches the given block" do
subject.cache_store! NaiveCache.new
driver = NaiveCache.new

subject.cache! :cache_key do |cache|
cache.cache "miss"
end
# cache miss
builder = described_class.new
builder.cache_store! driver

subject.cache! :cache_key do |cache|
builder.cache! :cache_key do |cache|
cache.cache "hit"
end

expect(subject.data!).to eq(cache: "miss")
expect(builder.data!).to eq(cache: "hit")

# cache hit
builder = described_class.new
builder.cache_store! driver

builder.cache! :cache_key do |cache|
cache.cache "miss"
end

expect(builder.data!).to eq(cache: "hit")
end
end

Expand Down
Loading

0 comments on commit 8891ea1

Please sign in to comment.