Skip to content

Cached respond to#3873

Closed
tenderlove wants to merge 2 commits into
ruby:masterfrom
Shopify:cached-respond-to
Closed

Cached respond to#3873
tenderlove wants to merge 2 commits into
ruby:masterfrom
Shopify:cached-respond-to

Conversation

@tenderlove
Copy link
Copy Markdown
Member

Use the pCMC for calls to respond_to?. This patch only works with "positive" cases (cases where respond_to? will return true).

This change makes the true case of respond to about 8x faster but the difference depends on the depth of the module:

[aaron@tc-lan-adapter ~/g/ruby (cached-respond-to)]$ make benchmark ITEM='respond_to'
/Users/aaron/.rubies/ruby-trunk/bin/ruby --disable=gems -rrubygems -I./benchmark/lib ./benchmark/benchmark-driver/exe/benchmark-driver \
	            --executables="compare-ruby::/Users/aaron/.rubies/ruby-trunk/bin/ruby --disable=gems -I.ext/common --disable-gem" \
	            --executables="built-ruby::./miniruby -I./lib -I. -I.ext/common  ./tool/runruby.rb --extout=.ext  -- --disable-gems --disable-gem" \
	            --output=markdown --output-compare $(find ./benchmark -maxdepth 1 -name 'respond_to' -o -name '*respond_to*.yml' -o -name '*respond_to*.rb' | sort) 
# Iteration per second (i/s)

|                  |compare-ruby|built-ruby|
|:-----------------|-----------:|---------:|
|respond_to_false  |      3.556M|    3.383M|
|                  |       1.05x|         -|
|respond_to_true   |      3.929M|   33.893M|
|                  |           -|     8.63x|

This change makes respond_to use the per class method cache
Comment thread vm_method.c
const rb_method_entry_t *me;
if (RB_TYPE_P(klass, T_CLASS) || RB_TYPE_P(klass, T_ICLASS)) {
me = (const rb_method_entry_t*)callable_method_entry(klass, id, defined_class_ptr);
} else {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which case on this else path?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure. I haven't had time to investigate why we need this else case :(

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the case for the else path. Ruby code like this:

module Foo
  module_function def omgomg; end
  instance_method(:omgomg) # called here
end

Here is the debugging info:

(lldb) bt
* thread #1, name = 'miniruby', stop reason = signal SIGABRT
  * frame #0: 0x00007ffff7c3818b libc.so.6`raise + 203
    frame #1: 0x00007ffff7c17859 libc.so.6`abort + 299
    frame #2: 0x0000555555642e19 miniruby`die at error.c:737:5
    frame #3: 0x0000555555642e0b miniruby`rb_bug(fmt="hello") at error.c:760:5
    frame #4: 0x00005555558b8894 miniruby`method_entry_resolve_refinement(klass=0x0000555555a7a940, id=30705, with_refinement=1, defined_class_ptr=0x00007fffffffb830) at vm_method.c:1131:9
    frame #5: 0x00005555558b85aa miniruby`rb_method_entry_with_refinements(klass=0x0000555555a7a940, id=30705, defined_class_ptr=0x00007fffffffb830) at vm_method.c:1156:12
    frame #6: 0x000055555577b09d miniruby`mnew(klass=0x0000555555a7a940, obj=0x0000000000000034, id=30705, mclass=0x0000555555ad2050, scope=0) at proc.c:1662:14
    frame #7: 0x0000555555778b5e miniruby`rb_mod_instance_method(mod=0x0000555555a7a940, vid=0x000000000077f10c) at proc.c:2068:12
    frame #8: 0x00005555558e6830 miniruby`ractor_safe_call_cfunc_1(recv=0x0000555555a7a940, argc=1, argv=0x00007ffff7808060, func=(miniruby`rb_mod_instance_method at proc.c:2063)) at vm_insnhelper.c:2645:12
    frame #9: 0x00005555558e1ae8 miniruby`vm_call_cfunc_with_frame(ec=0x0000555555a55520, reg_cfp=0x00007ffff7907f68, calling=0x00007fffffffbc40, cd=0x0000555555b76aa0) at vm_insnhelper.c:2821:11
    frame #10: 0x00005555558da43d miniruby`vm_call_cfunc(ec=0x0000555555a55520, reg_cfp=0x00007ffff7907f68, calling=0x00007fffffffbc40, cd=0x0000555555b76aa0) at vm_insnhelper.c:2842:12
    frame #11: 0x00005555558d9d17 miniruby`vm_call_method_each_type(ec=0x0000555555a55520, cfp=0x00007ffff7907f68, calling=0x00007fffffffbc40, cd=0x0000555555b76aa0) at vm_insnhelper.c:3324:16
    frame #12: 0x00005555558d9795 miniruby`vm_call_method(ec=0x0000555555a55520, cfp=0x00007ffff7907f68, calling=0x00007fffffffbc40, cd=0x0000555555b76aa0) at vm_insnhelper.c:3417:20
    frame #13: 0x00005555558bc35d miniruby`vm_call_general(ec=0x0000555555a55520, reg_cfp=0x00007ffff7907f68, calling=0x00007fffffffbc40, cd=0x0000555555b76aa0) at vm_insnhelper.c:3464:12
    frame #14: 0x00005555558d0b20 miniruby`vm_sendish(ec=0x0000555555a55520, reg_cfp=0x00007ffff7907f68, cd=0x0000555555b76aa0, block_handler=0x0000000000000000, method_explorer=(miniruby`vm_search_method_wrap at vm_insnhelper.c:4356)) at vm_insnhelper.c:4412:11
    frame #15: 0x00005555558a4126 miniruby`vm_exec_core(ec=0x0000555555a55520, initial=0x0000000000000000) at insns.def:789:11
    frame #16: 0x00005555558c633c miniruby`rb_vm_exec(ec=0x0000555555a55520, mjit_enable_p=1) at vm.c:2165:22
    frame #17: 0x00005555558c74a0 miniruby`rb_iseq_eval_main(iseq=0x0000555555a7ab20) at vm.c:2422:11
    frame #18: 0x000055555565093b miniruby`rb_ec_exec_node(ec=0x0000555555a55520, n=0x0000555555a7ab20) at eval.c:317:2
    frame #19: 0x0000555555650782 miniruby`ruby_run_node(n=0x0000555555a7ab20) at eval.c:375:30
    frame #20: 0x000055555557d358 miniruby`main(argc=2, argv=0x00007fffffffe518) at main.c:50:9
    frame #21: 0x00007ffff7c190b3 libc.so.6`__libc_start_main + 243
    frame #22: 0x000055555557d21e miniruby`_start + 46
(lldb) f 4
frame #4: 0x00005555558b8894 miniruby`method_entry_resolve_refinement(klass=0x0000555555a7a940, id=30705, with_refinement=1, defined_class_ptr=0x00007fffffffb830) at vm_method.c:1131:9
   1128	    if (RB_TYPE_P(klass, T_CLASS) || RB_TYPE_P(klass, T_ICLASS)) {
   1129	        me = (const rb_method_entry_t*)callable_method_entry(klass, id, defined_class_ptr);
   1130	    } else {
-> 1131	        rb_bug("hello");
   1132	        me = search_method_protect(klass, id, defined_class_ptr);
   1133	    }
   1134	
(lldb) command script import -r misc/lldb_cruby.py
lldb scripts for ruby has been installed.
(lldb) rp klass
bits [     ]
T_MODULE: (struct RClass) $1 = {
  basic = (flags = 0x0000000100000043, klass = 0x0000555555a7a8a0)
  super = 0x0000000000000000
  ptr = 0x0000555555b8de60
  class_serial = 577
}
(lldb) 

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to use the cme in place of the me? To fix respond_to? we just need to fix this location:

ruby/vm_method.c

Line 1359 in 383d3c3

me = method_entry_resolve_refinement(klass, id, TRUE, NULL);

@ko1 should I introduce a new method for use inside rb_method_boundp?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new method? new function?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cme is specialized version of me, so cme is me (me is sometimes cme, but sometimes not).

@ko1
Copy link
Copy Markdown
Contributor

ko1 commented Dec 14, 2020

#3899 should be better

@tenderlove
Copy link
Copy Markdown
Member Author

@ko1 thanks!

@tenderlove tenderlove closed this Dec 14, 2020
@tenderlove tenderlove deleted the cached-respond-to branch December 14, 2020 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants