[ruby-core:98923] [Ruby master Feature#16897] General purpose memoizer in Ruby 3 with Ruby 2 performance

Issue #16897 has been updated by jeremyevans0 (Jeremy Evans).


sam.saffron (Sam Saffron) wrote in #note-26:
> > In terms of usability, matz seems to like handling *args, **kwargs because it is explicit and not so complex. 
> 
> To me the design we arrived at is very very non-intuitive sadly, @matz 
> 
> ```
> def bar(a: 1)
> end
> 
> def foo(*x)
>   puts x
>   bar(*x)
> end
> ```
> 
> `*x` captures both kwargs and args, yet is is not allowed to pass kwargs along. 
> 
> 
> `foo(a: 1)` will print `{a: 1}` and then error out. This is very bad usability in the language.

The alternative is breaking compatibility with code that never used keyword arguments. See #14183 for the quite long discussion regarding this.
 
> `foo(a: 1)` should throw an exception cause it only captures args an not kwargs. At least that would guide people at the right direction.

This breaks backwards compatibility.  If people think migrating is hard with the changes we made in 2.7/3.0, I think they don't really understand how much harder it would have been if we stopped caller-side keywords from being converted to positional hashes (which dates back at least to Ruby 1.6, and probably earlier).

> My preference remains :
> 
> 1. Best... fix *x so it is able to delegate kwargs properly 100% of the time, `foo(a: 1)` should work, `foo({a: 1})` should exception. This means we codify and make official `{}.kwargs?` or something like that.

You can already get that behavior if you want, using `def foo(*args, **nil)`.  That is new syntax added in Ruby 2.7, specifically for people that do not want keyword to positional argument conversion.

You can also implement automatic keyword passing by default if you don't want to call `ruby2_keywords` for each method using `*args` where you want to pass keywords implicitly:

```ruby
class Module
  def method_added(method_name)
    meth = instance_method(method_name)
    return unless meth.source_location
    has_rest = false
    meth.parameters.each do |param|
      case param[0]
      when :key, :key_req, :keyrest, :no_key
        return
      when :rest
        has_rest = true
      end
    end

    if has_rest
      ruby2_keywords method_name
    end
  end
end
```

> But the current status quo is a huge trap we are leaving for future Ruby generations.

I disagree.  In my opinion, it's at most a minor issue.  It's far better not to break tons of code now.

The breakage for methods that accept keywords was necessary to fix the issues with keywords.  There is no need to change methods that do not accept keywords, since they didn't experience any of those issues.

Even if you consider the current design a mistake, in your example, with the master branch, you get an ArgumentError, which is straightforward to fix.  I'm not sure why you consider that a "huge trap".

----------------------------------------
Feature #16897: General purpose memoizer in Ruby 3 with Ruby 2 performance
https://bugs.ruby-lang.org/issues/16897#change-86301

* Author: sam.saffron (Sam Saffron)
* Status: Open
* Priority: Normal
----------------------------------------
```ruby
require 'benchmark/ips'

module Memoizer
def memoize_26(method_name)
  cache = {}

  uncached = "#{method_name}_without_cache"
  alias_method uncached, method_name

  define_method(method_name) do |*arguments|
    found = true
    data = cache.fetch(arguments) { found = false }
    unless found
      cache[arguments] = data = public_send(uncached, *arguments)
    end
    data
  end
end

  def memoize_27(method_name)
    cache = {}

    uncached = "#{method_name}_without_cache"
    alias_method uncached, method_name

    define_method(method_name) do |*args, **kwargs|
      found = true
      all_args = [args, kwargs]
      data = cache.fetch(all_args) { found = false }
      unless found
        cache[all_args] = data = public_send(uncached, *args, **kwargs)
      end
      data
    end
  end

  def memoize_27_v2(method_name)
    uncached = "#{method_name}_without_cache"
    alias_method uncached, method_name

    cache = "MEMOIZE_#{method_name}"

    params = instance_method(method_name).parameters
    has_kwargs = params.any? {|t, name| "#{t}".start_with? "key"}
    has_args = params.any? {|t, name| !"#{t}".start_with? "key"}

    args = []

    args << "args" if has_args
    args << "kwargs" if has_kwargs

    args_text = args.map do |n|
      n == "args" ? "*args" : "**kwargs"
    end.join(",")

    class_eval <<~RUBY
      #{cache} = {}
      def #{method_name}(#{args_text})
        found = true
        all_args = #{args.length === 2 ? "[args, kwargs]" : args[0]}
        data = #{cache}.fetch(all_args) { found = false }
        unless found
          #{cache}[all_args] = data = public_send(:#{uncached} #{args.empty? ? "" : ", #{args_text}"})
        end
        data
      end
    RUBY

  end

end

module Methods
  def args_only(a, b)
    sleep 0.1
    "#{a} #{b}"
  end

  def kwargs_only(a:, b: nil)
    sleep 0.1
    "#{a} #{b}"
  end

  def args_and_kwargs(a, b:)
    sleep 0.1
    "#{a} #{b}"
  end
end

class OldMethod
  extend Memoizer
  include Methods

  memoize_26 :args_and_kwargs
  memoize_26 :args_only
  memoize_26 :kwargs_only
end

class NewMethod
  extend Memoizer
  include Methods

  memoize_27 :args_and_kwargs
  memoize_27 :args_only
  memoize_27 :kwargs_only
end

class OptimizedMethod
  extend Memoizer
  include Methods

  memoize_27_v2 :args_and_kwargs
  memoize_27_v2 :args_only
  memoize_27_v2 :kwargs_only
end

OptimizedMethod.new.args_only(1,2)


methods = [
  OldMethod.new,
  NewMethod.new,
  OptimizedMethod.new
]

Benchmark.ips do |x|
  x.warmup = 1
  x.time = 2

  methods.each do |m|
    x.report("#{m.class} args only") do |times|
      while times > 0
        m.args_only(10, b: 10)
        times -= 1
      end
    end

    x.report("#{m.class} kwargs only") do |times|
      while times > 0
        m.kwargs_only(a: 10, b: 10)
        times -= 1
      end
    end

    x.report("#{m.class} args and kwargs") do |times|
      while times > 0
        m.args_and_kwargs(10, b: 10)
        times -= 1
      end
    end
  end

  x.compare!
end


# # Ruby 2.6.5
# #
# OptimizedMethod args only:   974266.9 i/s
#  OldMethod args only:   949344.9 i/s - 1.03x  slower
# OldMethod args and kwargs:   945951.5 i/s - 1.03x  slower
# OptimizedMethod kwargs only:   939160.2 i/s - 1.04x  slower
# OldMethod kwargs only:   868229.3 i/s - 1.12x  slower
# OptimizedMethod args and kwargs:   751797.0 i/s - 1.30x  slower
#  NewMethod args only:   730594.4 i/s - 1.33x  slower
# NewMethod args and kwargs:   727300.5 i/s - 1.34x  slower
# NewMethod kwargs only:   665003.8 i/s - 1.47x  slower
#
# #
# # Ruby 2.7.1
#
# OptimizedMethod kwargs only:  1021707.6 i/s
# OptimizedMethod args only:   955694.6 i/s - 1.07x  (0.00) slower
# OldMethod args and kwargs:   940911.3 i/s - 1.09x  (ア 0.00) slower
#  OldMethod args only:   930446.1 i/s - 1.10x  (ア 0.00) slower
# OldMethod kwargs only:   858238.5 i/s - 1.19x  (ア 0.00) slower
# OptimizedMethod args and kwargs:   773773.5 i/s - 1.32x  (ア 0.00) slower
# NewMethod args and kwargs:   772653.3 i/s - 1.32x  (ア 0.00) slower
#  NewMethod args only:   771253.2 i/s - 1.32x  (ア 0.00) slower
# NewMethod kwargs only:   700604.1 i/s - 1.46x  (ア 0.00) slower
```

The bottom line is that a generic delegator often needs to make use of all the arguments provided to a method.

```ruby
def count(*args, **kwargs)
  counter[[args, kwargs]] += 1
  orig_count(*args, **kwargs)
end
```

The old pattern meant we could get away with one less array allocation per:

```ruby
def count(*args)
  counter[args] += 1
  orig_count(*args, **kwargs)
end
```

I would like to propose some changes to Ruby 3 to allow to recover this performance. 

Perhaps:

```ruby
def count(...)
  args = ...
  counter[args] += 1
  orig_count(...)
end
```

Or:

```ruby
def count(***args)

  counter[args] += 1
  orig_count(***args)
end
```

Thoughts? 



-- 
https://bugs.ruby-lang.org/

Unsubscribe: <mailto:[e-mail]?subject=unsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>