Split cache keyword from method replacement

[smcgivern]

Currently, the cache keyword argument is used for 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 (https://github.com/exAspArk/batch-loader/blob/v1.3.0/lib/batch_loader.rb#L49-L55).
2. #__sync! uses it to replace methods on the proxied object with their 'real' equivalents (https://github.com/exAspArk/batch-loader/blob/v1.3.0/lib/batch_loader.rb#L77-L78).

In our use of this gem at GitLab, we've noticed that it's very easy for objects with a large interface to spend a lot of time in #__replace_with!. See https://gitlab.com/gitlab-org/gitlab-ce/issues/60373#note_159582633 and https://gitlab.com/gitlab-org/gitlab-ce/issues/43065#note_160469960 for some examples.

In some exploratory testing, we found that retaining only item 1 from my list above gave us better performance than either disabling the cache entirely, or having both 1 and 2 coupled together.

Could we consider a replace_methods keyword argument? If unset, it would default to the same value as cache, but if set to false, it would skip item 2 above. I am happy to create a PR if that sounds reasonable.

[smcgivern]

I took a look at a synthetic benchmark. All the numbers here are arbitrary but feel reasonable! I define two classes: one with 1,000 new methods (beyond those from Object), and one with a single new method.

Then, I do a very basic load: just replacing a value with itself. I do this 1,000 times for each case, and in each case, then call a method 1,000 times on the loaded object.

I tested this without caching on both objects, and with. I also added my proposed method_replacement: false keyword. Here's the script:

require 'benchmark'
require './lib/batch-loader'

class ManyMethods
  1.upto(1000) 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)
  1000.times do
    value = load_value(klass.new, opts)
    1000.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, method_replacement: false) }
  x.report('no replacement + few methods') { benchmark(klass: FewMethods, method_replacement: false) }
  x.report('no cache + many methods') { benchmark(klass: ManyMethods, cache: false, method_replacement: false) }
  x.report('no cache + few methods') { benchmark(klass: FewMethods, cache: false, method_replacement: false) }
end

And here are the results (for me):

Rehearsal -----------------------------------------------------------------
replacement + many methods      2.090000   0.020000   2.110000 (  2.297490)
replacement + few methods       0.420000   0.010000   0.430000 (  0.421501)
no replacement + many methods   0.410000   0.000000   0.410000 (  0.422869)
no replacement + few methods    0.360000   0.000000   0.360000 (  0.364277)
no cache + many methods        28.720000   0.070000  28.790000 ( 29.041651)
no cache + few methods         29.340000   0.100000  29.440000 ( 29.748892)
------------------------------------------------------- total: 61.540000sec

                                    user     system      total        real
replacement + many methods      2.200000   0.020000   2.220000 (  2.232047)
replacement + few methods       0.410000   0.000000   0.410000 (  0.418404)
no replacement + many methods   0.400000   0.000000   0.400000 (  0.407584)
no replacement + few methods    0.390000   0.000000   0.390000 (  0.395476)
no cache + many methods        29.300000   0.120000  29.420000 ( 29.765799)
no cache + few methods         30.610000   0.210000  30.820000 ( 31.551676)

I found it a bit suspicious that no replacement was always faster than caching, so I bumped it up to 10,000 calls on each loaded object, and I got (skipping the cache-less case as it's too slow):

Rehearsal -----------------------------------------------------------------
replacement + many methods      4.970000   0.010000   4.980000 (  5.000493)
replacement + few methods       2.860000   0.010000   2.870000 (  2.902864)
no replacement + many methods   3.440000   0.010000   3.450000 (  3.471370)
no replacement + few methods    3.250000   0.010000   3.260000 (  3.259099)
------------------------------------------------------- total: 14.560000sec

                                    user     system      total        real
replacement + many methods      4.810000   0.010000   4.820000 (  4.854519)
replacement + few methods       2.680000   0.010000   2.690000 (  2.693626)
no replacement + many methods   3.270000   0.000000   3.270000 (  3.279509)
no replacement + few methods    3.210000   0.010000   3.220000 (  3.225818)

Which shows that with an object with a lot of methods, the technique of replacing the methods can be slower than #method_missing. For an object with fewer methods, we still need a lot of method calls to notice the difference.

@exAspArk what do you think? The actual patch so far is just:

diff --git a/lib/batch_loader.rb b/lib/batch_loader.rb
index ac1638c..f2e2e29 100644
--- a/lib/batch_loader.rb
+++ b/lib/batch_loader.rb
@@ -23,10 +23,11 @@ class BatchLoader
     @__executor_proxy = executor_proxy
   end

-  def batch(default_value: nil, cache: true, key: nil, &batch_block)
+  def batch(default_value: nil, cache: true, key: nil, method_replacement: true, &batch_block)
     @default_value = default_value
     @cache = cache
     @key = key
+    @method_replacement = method_replacement
     @batch_block = batch_block
     __executor_proxy.add(item: @item)

@@ -74,7 +75,7 @@ class BatchLoader
   def __sync!
     loaded_value = __sync

-    if @cache
+    if @method_replacement
       __replace_with!(loaded_value)
     else
       loaded_value

[stanhu]

http://franck.verrot.fr/blog/2015/07/12/benchmarking-ruby-method-missing-and-define-method (although written in 2015) seems to suggest that define_method should be faster than method_missing. The problem may be that the public_send in BatchLoader in

batch-loader/lib/batch_loader.rb

Line 122 in 43e4f62

value.public_send(method_name, *args, &block)

is negating the benefit here.

[smcgivern]

Yes, I think that implementation will be faster over many calls, but maybe not if there are not so many. Having this configurable by the users of the library would be nice so that we can experiment, anyway 🙂

[exAspArk]

Hey @smcgivern and @stanhu,

Thanks a lot for opening the issue and diving into the performance optimizations! Does this approach help to solve the mentioned GitLab issues?

Will be amazing if you could open a PR. Maybe we could rename the flag to replace_methods:

foo = BatchLoader.for(bar).batch(replace_methods: true) { ... }

From your benchmarks, using the flag doesn't always improve the performance. I guess it primarily depends on 2 factors:

• How many methods an object has
• How many method calls the object will receive. Whether it's faster to replace them immediately or use method_missing with "pay as you go" 😸

We could also update the existing synthetic benchmarks https://github.com/exAspArk/batch-loader/tree/master/spec/benchmarks so other devs could check before and after their changes locally, for example :)

[smcgivern]

How many method calls the object will receive. Whether it's faster to replace them immediately or use method_missing with "pay as you go" 😸

Exactly that! I don't want to make a decision for people here; I'm sure that within the GitLab application, there are cases where replace_methods would speed things up, and cases where it would slow them down.

I've created #45 now, thanks for the pointers!