From: "alanwu (Alan Wu) via ruby-core" Date: 2023-02-27T21:36:29+00:00 Subject: [ruby-core:112622] [Ruby master Bug#18572] Performance regression when invoking refined methods 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/ ______________________________________________ ruby-core mailing list -- ruby-core@ml.ruby-lang.org To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org ruby-core info -- https://ml.ruby-lang.org/mailman3/postorius/lists/ruby-core.ml.ruby-lang.org/