Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip ancestry check and allow method caching for protected FCALLs #5643

Merged
merged 3 commits into from
Jun 22, 2022

Conversation

jhawthorn
Copy link
Member

@jhawthorn jhawthorn commented Mar 11, 2022

If we are making an FCALL, we know we are calling a method on self. This is the same check made for private method visibility, so I think it should also guarantee we can call the protected method. (Hopefully I haven't missed some case the check was needed)

For example in the following foo we should not have to check the ancestry of the bar receiver, as we know it is self:

class Foo
  def foo
    bar
  end

  protected
  def bar
    123
  end
end

This allows us to skip the ancestry check on the receiver (which was recently made faster, but still not as fast as skipping the check entirely 😁). And allows setting the fastpath to the inline method cache in the interpreter.

I don't think this pattern is particularly common, and probably most often found in code that should be using private, but there are some proper uses (when a method is sometimes called via fcall and sometimes on a different receiver). Fortunately I think this optimization is essentially free, as we were already checking flags everywhere this PR changes.

Benchmark
prelude: |
  class BaseClass
    def call_public
      public_foo
      public_foo
      public_foo
      public_foo
      public_foo
      public_foo
      public_foo
      public_foo
      public_foo
      public_foo
    end
    def call_self
      foo
      foo
      foo
      foo
      foo
      foo
      foo
      foo
      foo
      foo
    end
    def call_other(other)
      other.foo
      other.foo
      other.foo
      other.foo
      other.foo
      other.foo
      other.foo
      other.foo
      other.foo
      other.foo
    end
    def public_foo
      123
    end
    protected
    def foo
      123
    end
  end
  class MyClass < BaseClass
    10.times { include Module.new }
  end
  OBJ = MyClass.new
benchmark:
  call_public: |
    OBJ.call_public
  call_self: |
    OBJ.call_self
  call_other: |
    OBJ.call_other(OBJ)
loop_count: 2000000

Interpreter:

|             |Ruby 3.1|   trunk|This branch|
|:------------|-------:|-------:|----------:|
|call_public  |  6.216M|  6.303M|     6.337M|
|             |       -|   1.01x|      1.02x|
|call_self    |  2.292M|  2.973M|     6.357M|
|             |       -|   1.30x|      2.77x|
|call_other   |  2.349M|  2.945M|     2.973M|
|             |       -|   1.25x|      1.27x|

YJIT:

|             |Ruby 3.1|   trunk|This branch|
|:------------|-------:|-------:|----------:|
|call_public  | 18.945M| 18.873M|    19.575M|
|             |   1.00x|       -|      1.04x|
|call_self    |  6.454M| 13.706M|    19.452M|
|             |       -|   2.12x|      3.01x|
|call_other   |  6.535M| 13.234M|    13.449M|
|             |       -|   2.02x|      2.06x|

Comment on lines +256 to +260
static inline bool
vm_call_cacheable(const struct rb_callinfo *ci, const struct rb_callcache *cc)
{
return (vm_ci_flag(ci) & VM_CALL_FCALL) ||
METHOD_ENTRY_VISI(vm_cc_cme(cc)) != METHOD_VISI_PROTECTED;
}
Copy link
Contributor

@maximecb maximecb Mar 11, 2022

Choose a reason for hiding this comment

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

Imo you should add some comments here to explain what this method checks. What does it mean for a call to be cacheable? Why is that so? Makes the code more approachable for people not familiar with the Ruby codebase. Not trying to be negative, it's just Ruby is a big language and this is a big codebase.

yjit_codegen.c Outdated
@@ -4137,7 +4137,9 @@ gen_send_general(jitstate_t *jit, ctx_t *ctx, struct rb_call_data *cd, rb_iseq_t
}
break;
case METHOD_VISI_PROTECTED:
jit_protected_callee_ancestry_guard(jit, cb, cme, side_exit);
if (!(vm_ci_flag(ci) & (VM_CALL_OPT_SEND | VM_CALL_FCALL))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could also comment here as to what flags are being checked and why.

@maximecb
Copy link
Contributor

@noahgibbs if this PR is merged, we'll want to forward port this change into RustYJIT. Though we'd have to rebase on upstream CRuby first 🤔

@maximecb
Copy link
Contributor

Actually @jhawthorn, would you want to implement this change in RustYJIT? :) Could give you a quick tour of the codebase if you have time next week.

@XrXr
Copy link
Member

XrXr commented Mar 11, 2022

# For 89d47d11e37efa0aaf767cafbd69e15bb0a42707, GH-5643
# The PR changes behavior for protected refinement methods. 
#
# In a world without refinements, I think this is sound because FCALL
# means we did method lookup on `self` and the ancestry check
# and method lookup psudo algo both scan the same ancestors list in the same way.
# Finding the method implies ancestry.

class A
end

module RefProt
  refine(A) { protected def foo = :refined }
end

class A
  using RefProt

  def foo
    :normal
  end

  def call_foo
    foo
  end
end

A.new.call_foo

Sorry.

@jhawthorn
Copy link
Member Author

Thanks for the test Alan. I was worried there would be an edge case in this. I think we could easily test for a refinement, but that might make the added complexity not worth it. I'll re-evaluate (and see how often real code might hit this).

If I do decide to move forward with this, of course I'm happy to implement in RustYJIT 🙂

@jhawthorn jhawthorn marked this pull request as draft May 25, 2022 23:18
@jhawthorn
Copy link
Member Author

With the protected/refinement behaviour clarified in https://bugs.ruby-lang.org/issues/18806, and a fix pending in #5966 I think this can move forward.

I've also added a commit applying the optimization to the Rust YJIT codegen.

If we are making an FCALL, we know we are calling a method on self. This
is the same check made for private method visibility, so it should also
guarantee we can call a protected method.
@jhawthorn jhawthorn marked this pull request as ready for review June 16, 2022 20:29
@jhawthorn jhawthorn merged commit 0c1f643 into ruby:master Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants