Add replace_methods argument to BatchLoader#batch

The existing `cache` argument does two things:

1. `#__sync` uses it to set a `@synced` instance variable, which tells
   later calls to `#__sync` not to reload the value when another method is
   called on it.
2. `#__sync!` uses it to replace methods on the proxied object with
   their 'real' equivalents.

This delegates responsibility for item 2 to the new `replace_methods`
argument instead. In testing (and in the benchmarks added here) we've
seen that `#__replace_with!` can actually take up an appreciable amount
of time, and removing it is faster, even though each individual method
call is slower.

The default for this new argument is to match `cache`. Only if it's
explicitly set to `true` or `false` will it not do that, and if it is
set to `true` and `cache` is `false` we raise an error, because that
doesn't make sense.

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,16 @@ puts user_lazy(1) # SELECT * FROM users WHERE id IN (1)
 puts user_lazy(1) # SELECT * FROM users WHERE id IN (1)
 ```
 
+### 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 +404,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` | `nil`                                                                | Defaults to the value of `cache`. 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,18 @@ 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: nil, key: nil, &batch_block)
     @default_value = default_value
     @cache = cache
+    # Default to the value of `cache` unless explicitly set
+    @replace_methods = replace_methods.nil? ? @cache : replace_methods
     @key = key
     @batch_block = batch_block
+
+    if @replace_methods && !@cache
+      raise ArgumentError, "Can't replace methods without caching the value"
+    end
+
     __executor_proxy.add(item: @item)
 
     __singleton_class.class_eval { undef_method(:batch) }
@@ -74,7 +81,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 +93,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

@@ -284,12 +284,27 @@
       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
       # require 'pry'; binding.pry
       expect { result.to_s }.to raise_error("Oops")
       expect { result.to_s }.to raise_error("Oops")
     end
+
+    it 'raises an error when replace_methods is set and cache is not' do
+      expect { BatchLoader.for(1).batch(cache: false, replace_methods: true) { 1 } }.
+        to raise_error(ArgumentError)
+    end
   end
 end

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