[ruby-core:112622] [Ruby master Bug#18572] Performance regression when invoking refined methods

27 Feb 2023

      Issue #18572 has been updated by alanwu (Alan Wu).

Yes, the perf regression is unfortunate.

The current per-call-site cache (`CC`) in the interpreter is keyed on
`[recever_class, call_target_method_entry_validity]` -- the lexical scope that
is used to resolve refinement definition for each call site is *not* cached. The
way the system resolves calls to refinement definitions is through a special
type of method entry (`VM_METHOD_TYPE_REFINED`) that triggers a secondary
uncached lookup which takes into account the lexical scope of the call site. Any
refinement definition for a module/class `M`, even ones that are never
activated through `Module#using`, makes all calls to that method name in `M`
do this uncached secondary lookup. This is the issue the OP showed in the
`no-op {original,refined}` benchmark pair.

The fact that there is a full heap walk during every call to `Module#using` is
a separate issue. If you believe what I said about the secondary lookup always
happening for refined method, and believe that the secondary lookup is always
uncached, then this heap walk to invalidate all `CC`s should be unnecessary.
This was what I noticed when I sent https://github.com/ruby/ruby/pull/4323 in
2021. The change itself is simple and we can merge it if we can get consensus
that my assertions are correct.

Reworking the cache system to fix the regression for the benchmark in the OP
would be much more invovled.
...
Maybe we should deprecate refinements: my main concern about them is it's a
ton of complexity in Ruby implementations ...
Their semantics definitely make the minimum required implementation complexity fairly
high, and make cach design tricky. Even the current relatively simple cache
design had bugs (e.g. #17806) when it first landed. I don't think they should
be deprecated, though, since they've been around long enough that removing them
would make many users unhappy. However, I would advise against introducing _any_ new
additional complications to their semantics (e.g. #16461, #19277). We've already seen
how this kind of complexity can make it more challenging for new developments like
Ractors and YJIT.

----------------------------------------
Bug #18572: Performance regression when invoking refined methods
https://bugs.ruby-lang.org/issues/18572#change-102070

* Author: palkan (Vladimir Dementyev)
* Status: Assigned
* Priority: Normal
* Assignee: ko1 (Koichi Sasada)
* Backport: 2.6: UNKNOWN, 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN
----------------------------------------
Since Ruby 3.0, defining a refinement for a method slows down its execution even if we do not activate the refinement:

```ruby
require "benchmark_driver"

source = <<~RUBY
class Hash
  def symbolize_keys
    transform_keys { |key| key.to_sym rescue key }
  end
  def refined_symbolize_keys
    transform_keys { |key| key.to_sym rescue key }
  end
end
module HashRefinements
  refine Hash do
    def refined_symbolize_keys
      raise "never called"
    end
  end
end
HASH = {foo: 1, bar: 2, baz: 3}
class Foo
  def original
  end
  def refined
  end
end
module FooRefinements
  refine Foo do
    def refined
      raise "never called"
    end
  end
end
FOO = Foo.new
RUBY

Benchmark.driver do |x|
  x.prelude %Q{
    #{source}
  }
  x.report "#symbolize_keys original", %{ HASH.symbolize_keys }
  x.report "#symbolize_keys refined", %{ HASH.refined_symbolize_keys }
end

Benchmark.driver do |x|
  x.prelude %Q{
    #{source}
  }
  x.report "no-op original", %{ FOO.original }
  x.report "no-op refined", %{ FOO.refined }
end
```

The results for Ruby 3.1:

```sh
...

Comparison:
#symbolize_keys original:   2372420.1 i/s 
 #symbolize_keys refined:   1941019.0 i/s - 1.22x  slower

...

Comparison:
      no-op original:  51790974.2 i/s 
       no-op refined:  14456518.9 i/s - 3.58x  slower
```

For Ruby 2.6 and 2.7:

```sh
Comparison:
#symbolize_keys original:   2278339.7 i/s 
 #symbolize_keys refined:   2264153.1 i/s - 1.01x  slower

...

Comparison:
       no-op refined:  64178338.5 i/s 
      no-op original:  63357980.1 i/s - 1.01x  slower
```

You can find the full code and more results in this [gist](https://gist.github.com/palkan/637dc83edd86d70b5dbf72f2a4d702e5).

P.S.  The problem was originally noticed by @byroot, see https://github.com/ruby-i18n/i18n/pull/573

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