Add support for memoized methods with block arguments

[oleander]

See #46.

Two calls to a memoized method with different block arguments will now be cached separately.

[flash-gordon]

I think this is not practical. Every call of a memoized method normally creates a new block with a new hash value. The only way not to have a leak is to ensure only a finite amount of calls done or blocks are cached somewhere else. I don't think this API is worth adding.

[oleander]

@flash-gordon

Every call of a memoized method normally creates a new block with a new hash value.

def test(&block)
  puts block.hash
end

2.times do
  test do
    # NOP
  end
end

Yields:

4375437158601092624
4375437158601092624

This one outputs the same hash value for both calls to test tho, or maybe I misunderstood what you meant. If the block's hash isn't part of the cache key then we should at least pass the block to super or raise an exception notifying the caller that blocks aren't supported ツ

[flash-gordon]

@oleander yeah, you're right. I wonder where my prev understanding is coming from... Anyway, this makes it practical for me :) Although, it's not possible to determine if a method accepts a block because it can use the yield keyword.

[flash-gordon]

I find this odd though I do understand how it works

def capture(&block)
  block
end

def wrap(value)
  capture { value }
end

f = wrap(1)
g = wrap(2)

puts(f.hash == g.hash) # => true
puts(f.() == g.())     # => false

[oleander]

The new implentation doesn't have to be perfect tho, I would argue—just better than the current one. If we can support explicitly declared blocks I would call it a win. Under the assumption that the change is backward compatible with the current version.

We could check for blocks using "block_given?" And check that a block is included in "method.parameters.to_h.key?(:block)". If it isn't then "yield" is used and we should fall back to the previous implementation, not cache the call or raise an error.

[solnic]

@oleander yeah, you're right. I wonder where my prev understanding is coming from

@flash-gordon actually, my initial gut-feel reaction whenever I see blocks and memoization is to panic, so I totally understand your initial concern! It seems like we're good here though

@oleander @flash-gordon I think it's worth adding and I also think it's worth having a generic API for redefining methods while preserving signatures. We use all sorts of method-overriding and method decoration in dry-rb and rom-rb ecosystems so this would be beneficial (assuming it's possible to have a stable implementation 🙂).

Also, to make sure we're on the same page:

Ah, I understand what you mean now. You want the memorized method to not only behave in the same way as the original one but also to return the same stats as the original, i.e trough method(:mymethod).parameters.

This is exactly what I meant, sorry if I rushed with my initial explanation! I believe this would avoid any potential confusion with overridden methods and also open up doors for some new possibilities where you may want to use parameters info to do...well, something cool 🤓

So, we could use it for memoization API here and if it works well, we could promote it to a 1st-class API that could be used in other gems.

[oleander]

Ran some benchmarks against master using this implementation based on module_eval (Ruby 2.5.1).

require "bundler/setup"
require "benchmark/ips"

Bundler.require

require 'dry/core/memoizable'

class Test
  include Dry::Core::Memoizable

  def call(*args, **kwargs)
    # NOP
  end

  memoize :call
end

test = Test.new
block = -> { }

Benchmark.ips do |x|
  x.report("new") { test.call(:arg, key: :value) }
end

Without block

master

Warming up --------------------------------------
                 new     3.627k i/100ms
Calculating -------------------------------------
                 new     37.082k (± 4.7%) i/s -    184.977k in   4.999907s

module_eval

Warming up --------------------------------------
                 new     3.636k i/100ms
Calculating -------------------------------------
                 new     35.813k (± 6.8%) i/s -    178.164k in   5.000445s

With block

master

Warming up --------------------------------------
                 new     2.876k i/100ms
Calculating -------------------------------------
                 new     28.488k (± 7.1%) i/s -    143.800k in   5.077245s

module_eval

Warming up --------------------------------------
                 new     2.854k i/100ms
Calculating -------------------------------------
                 new     28.937k (± 4.3%) i/s -    145.554k in   5.039862s

[oleander]

Rebased with master to add support for BasicObject.

[oleander]

we could promote it to a 1st-class API that could be used in other gems.

I didn't know that wasn't already the case. I've been using it outside the dry-suite for months :)

[solnic]

we could promote it to a 1st-class API that could be used in other gems. I didn't know that wasn't already the case. I've been using it outside the dry-suite for months :)

@oleander sorry, I meant the API for redefining methods while preserving original sigs! Memoizable is a 1st-class, public API. We should have it documented one day 😓 I want to wrap up dry-core 1.0.0 soon so this will be a good moment to revamp the docs.

[flash-gordon]

As I'm adding specs for this it turned out Proc#hash can return different values for the same block:

def capture(&block)
  block.hash
end

ids = Array.new(1000) { capture { :foo } }.uniq

puts(ids.size) # this returns different values on each call

This leads to a memory leak. I didn't look into details yet but my gut feelings tell me we won't be able to "fix" this. In this case, I would suggest deprecating using blocks for memorization. It's still fine to pass them, it's just Proc#hash is unreliable for our purposes.

[flash-gordon]

will wait for comments on https://bugs.ruby-lang.org/issues/17951

[oleander]

ops :/

@flash-gordon Thanks for posting the ticket link. I'm curious to see what they say