Merge pull request #45 from smcgivern/add-replace-methods-argument

Add `replace_methods` argument to BatchLoader#batch

CHANGELOG.md

@@ -10,7 +10,7 @@ that you can set version constraints properly.
 
 #### [Unreleased](https://github.com/exAspArk/batch-loader/compare/v1.3.0...HEAD)
 
-* WIP
+* `Added`: new `replace_methods` argument to `BatchLoader#batch` to allow control over `define_method` calls
 
 #### [v1.3.0](https://github.com/exAspArk/batch-loader/compare/v1.2.2...v1.3.0)
 

README.md

@@ -20,6 +20,7 @@ This gem provides a generic lazy batching mechanism to avoid N+1 DB queries, HTT
   * [Loading multiple items](#loading-multiple-items)
   * [Batch key](#batch-key)
   * [Caching](#caching)
+  * [Replacing methods](#replacing-methods)
 * [Installation](#installation)
 * [API](#api)
 * [Implementation details](#implementation-details)
@@ -374,6 +375,18 @@ puts user_lazy(1) # SELECT * FROM users WHERE id IN (1)
 puts user_lazy(1) # SELECT * FROM users WHERE id IN (1)
 ```
 
+If you set `cache: false`, it's likely you also want `replace_methods: false` (see below section).
+
+### Replacing methods
+
+By default, using the cache will also replace methods on the `BatchLoader` instance by calling `#define_method` to copy methods from the loaded value. In some cases, this can be slower than using the naive `#method_missing` implementation when caching is disabled. To keep caching but disable method replacement, set:
+
+```ruby
+BatchLoader.for(id).batch(cache: true, replace_methods: false) do |ids, loader|
+  # ...
+end
+```
+
 ## Installation
 
 Add this line to your application's Gemfile:
@@ -393,20 +406,23 @@ Or install it yourself as:
 ## API
 
 ```ruby
-BatchLoader.for(item).batch(default_value: default_value, cache: cache, key: key) do |items, loader, args|
+BatchLoader
+  .for(item)
+  .batch(default_value: default_value, cache: cache, replace_methods: replace_methods, key: key) do |items, loader, args|
   # ...
 end
 ```
 
-| Argument Key    | Default                                       | Description                                                   |
-| --------------- | --------------------------------------------- | ------------------------------------------------------------- |
-| `item`          | -                                             | Item which will be collected and used for batching.           |
-| `default_value` | `nil`                                         | Value returned by default after batching.                     |
-| `cache`         | `true`                                        | Set `false` to disable caching between the same executions.   |
-| `key`           | `nil`                                         | Pass custom key to uniquely identify the batch block.         |
-| `items`         | -                                             | List of collected items for batching.                         |
-| `loader`        | -                                             | Lambda which should be called to load values loaded in batch. |
-| `args`          | `{default_value: nil, cache: true, key: nil}` | Arguments passed to the `batch` method.                       |
+| Argument Key      | Default                                                              | Description                                                                            |
+| ---------------   | ---------------------------------------------                        | -------------------------------------------------------------                          |
+| `item`            | -                                                                    | Item which will be collected and used for batching.                                    |
+| `default_value`   | `nil`                                                                | Value returned by default after batching.                                              |
+| `cache`           | `true`                                                               | Set `false` to disable caching between the same executions.                            |
+| `replace_methods` | `true`                                                               | Set `false` to always use `#method_missing` to respond to methods on the loaded value. |
+| `key`             | `nil`                                                                | Pass custom key to uniquely identify the batch block.                                  |
+| `items`           | -                                                                    | List of collected items for batching.                                                  |
+| `loader`          | -                                                                    | Lambda which should be called to load values loaded in batch.                          |
+| `args`            | `{default_value: nil, cache: true, replace_methods: true, key: nil}` | Arguments passed to the `batch` method.                                                |
 
 ## Implementation details
 

lib/batch_loader.rb

@@ -23,11 +23,13 @@ def initialize(item:, executor_proxy: nil)
     @__executor_proxy = executor_proxy
   end
 
-  def batch(default_value: nil, cache: true, key: nil, &batch_block)
+  def batch(default_value: nil, cache: true, replace_methods: true, key: nil, &batch_block)
     @default_value = default_value
     @cache = cache
+    @replace_methods = replace_methods
     @key = key
     @batch_block = batch_block
+
     __executor_proxy.add(item: @item)
 
     __singleton_class.class_eval { undef_method(:batch) }
@@ -74,7 +76,7 @@ def method_missing(method_name, *args, &block)
   def __sync!
     loaded_value = __sync
 
-    if @cache
+    if @replace_methods
       __replace_with!(loaded_value)
     else
       loaded_value
@@ -86,7 +88,7 @@ def __ensure_batched
 
     items = __executor_proxy.list_items
     loader = __loader
-    args = {default_value: @default_value, cache: @cache, key: @key}
+    args = {default_value: @default_value, cache: @cache, replace_methods: @replace_methods, key: @key}
     @batch_block.call(items, loader, args)
     items.each do |item|
       next if __executor_proxy.value_loaded?(item: item)

spec/batch_loader_spec.rb

@@ -239,9 +239,9 @@
       expect(subject).to respond_to(:id)
     end
 
-    context 'when the cache is disabled' do
+    context 'when the cache and method replacement is disabled' do
       it 'syncs the object on every call' do
-        loaded_user = post.user_lazy(cache: false)
+        loaded_user = post.user_lazy(cache: false, replace_methods: false)
 
         expect(User).to receive(:where).with(id: [1]).twice.and_call_original
 
@@ -273,17 +273,27 @@
       expect(post.user_lazy(cache: false)).to eq(user2)
     end
 
-    it 'works without cache for the same BatchLoader instance' do
+    it 'works without cache and method replacement for the same BatchLoader instance' do
       user = User.save(id: 1)
       post = Post.new(user_id: user.id)
-      user_lazy = post.user_lazy(cache: false)
+      user_lazy = post.user_lazy(cache: false, replace_methods: false)
 
       expect(User).to receive(:where).with(id: [1]).twice.and_call_original
 
       expect(user_lazy).to eq(user)
       expect(user_lazy).to eq(user)
     end
 
+    it 'does not replace methods when replace_methods is false' do
+      user = User.save(id: 1)
+      post = Post.new(user_id: user.id)
+      user_lazy = post.user_lazy(cache: true, replace_methods: false)
+
+      expect(user_lazy).to receive(:method_missing).and_call_original
+
+      user_lazy.id
+    end
+
     it 'raises the error if something went wrong in the batch' do
       result = BatchLoader.for(1).batch { |ids, loader| raise "Oops" }
       # should work event with Pry which currently shallows errors on #inspect call https://github.com/pry/pry/issues/1642

spec/benchmarks/caching.rb

@@ -0,0 +1,63 @@
+# Usage: ruby spec/benchmarks/caching.rb
+
+# no replacement + many methods _can_ be faster than replacement + many
+# methods, but this depends on the values of METHOD_COUNT, OBJECT_COUNT,
+# and CALL_COUNT, so tweak them for your own scenario!
+
+require 'benchmark'
+require_relative '../../lib/batch_loader'
+
+METHOD_COUNT = 1000 # methods on the object with a large interface
+OBJECT_COUNT = 1000 # objects in the batch
+CALL_COUNT = 1000 # times a method is called on the loaded object
+
+class ManyMethods
+  1.upto(METHOD_COUNT) do |i|
+    define_method("method_#{i}") { i }
+  end
+end
+
+class FewMethods
+  def method_1
+    1
+  end
+end
+
+def load_value(x, **opts)
+  BatchLoader.for(x).batch(opts) do |xs, loader|
+    xs.each { |x| loader.call(x, x) }
+  end
+end
+
+def benchmark(klass:, **opts)
+  OBJECT_COUNT.times do
+    value = load_value(klass.new, opts)
+    CALL_COUNT.times { value.method_1 }
+  end
+end
+
+Benchmark.bmbm do |x|
+  x.report('replacement + many methods') { benchmark(klass: ManyMethods) }
+  x.report('replacement + few methods') { benchmark(klass: FewMethods) }
+  x.report('no replacement + many methods') { benchmark(klass: ManyMethods, replace_methods: false) }
+  x.report('no replacement + few methods') { benchmark(klass: FewMethods, replace_methods: false) }
+  x.report('no cache + many methods') { benchmark(klass: ManyMethods, cache: false, replace_methods: false) }
+  x.report('no cache + few methods') { benchmark(klass: FewMethods, cache: false, replace_methods: false) }
+end
+
+# Rehearsal -----------------------------------------------------------------
+# replacement + many methods      2.260000   0.030000   2.290000 (  2.603038)
+# replacement + few methods       0.450000   0.000000   0.450000 (  0.457151)
+# no replacement + many methods   0.440000   0.010000   0.450000 (  0.454444)
+# no replacement + few methods    0.370000   0.000000   0.370000 (  0.380699)
+# no cache + many methods        31.780000   0.240000  32.020000 ( 33.552620)
+# no cache + few methods         31.510000   0.200000  31.710000 ( 32.294752)
+# ------------------------------------------------------- total: 67.290000sec
+
+#                                     user     system      total        real
+# replacement + many methods      2.330000   0.010000   2.340000 (  2.382599)
+# replacement + few methods       0.430000   0.000000   0.430000 (  0.438584)
+# no replacement + many methods   0.420000   0.000000   0.420000 (  0.434069)
+# no replacement + few methods    0.440000   0.010000   0.450000 (  0.452091)
+# no cache + many methods        31.630000   0.160000  31.790000 ( 32.337531)
+# no cache + few methods         36.590000   0.370000  36.960000 ( 40.701712)

spec/fixtures/models.rb

@@ -28,8 +28,8 @@ def initialize(user_id:, title: nil)
     self.title = title || "Untitled"
   end
 
-  def user_lazy(cache: true)
-    BatchLoader.for(user_id).batch(cache: cache) do |user_ids, loader|
+  def user_lazy(**opts)
+    BatchLoader.for(user_id).batch(**opts) do |user_ids, loader|
       User.where(id: user_ids).each { |user| loader.call(user.id, user) }
     end
   end