diff --git a/CHANGELOG.md b/CHANGELOG.md index 84c0a5f..81423cc 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/README.md b/README.md index 9b75959..b2831b2 100644 --- a/README.md +++ b/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 diff --git a/lib/batch_loader.rb b/lib/batch_loader.rb index ac1638c..b9032ea 100644 --- a/lib/batch_loader.rb +++ b/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) diff --git a/spec/batch_loader_spec.rb b/spec/batch_loader_spec.rb index 7e7f502..6e5f104 100644 --- a/spec/batch_loader_spec.rb +++ b/spec/batch_loader_spec.rb @@ -284,6 +284,16 @@ 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 @@ -291,5 +301,10 @@ 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 diff --git a/spec/benchmarks/caching.rb b/spec/benchmarks/caching.rb new file mode 100644 index 0000000..5df3b8d --- /dev/null +++ b/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) diff --git a/spec/fixtures/models.rb b/spec/fixtures/models.rb index b37c95e..6a45064 100644 --- a/spec/fixtures/models.rb +++ b/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