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

Ruby 2.7: Proc#<< and Proc#>> raises TypeError if passed not callable object #2164

Conversation

ssnickolay
Copy link
Collaborator

private

def check_if_callable!(value)
return if value.kind_of?(Proc) || value.kind_of?(Method) || value.respond_to?(:call)
Copy link
Member

@eregon eregon Nov 18, 2020

Choose a reason for hiding this comment

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

Both Proc and Method respond_to?(:call) and respond_to? inline caches in TruffleRuby, so there is no need to check explicit types here.

Given it's so simple after that, I would skip the helper and have the logic inline raise unless ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm, I thought that anyway kind_of? is a bit faster than respond_to? even if the method is inlined. Or it is not so significant?

Copy link
Member

Choose a reason for hiding this comment

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

I would think both compile down to a single check on the metaclass.
You can verify with IGV or https://github.com/Shopify/seafoam if you like, or just write a benchmark using benchmark-ips :)

Copy link
Member

Choose a reason for hiding this comment

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

require 'benchmark/ips'

VALUE = -> {}

Benchmark.ips do |x|
  x.report 'kind_of?' do
    VALUE.kind_of?(Proc)
  end
  
  x.report 'respond_to?' do
    VALUE.respond_to?(:call)
  end
end
ruby bench_kind_of_respond_to.rb
Warming up --------------------------------------
            kind_of?   415.314M i/100ms
         respond_to?   434.209M i/100ms
Calculating -------------------------------------
            kind_of?      4.246B (± 1.7%) i/s -     21.596B in   5.088181s
         respond_to?      4.224B (± 0.4%) i/s -     21.276B in   5.036533s

So they both seem to optimize away.

Also more checks would be bad if the value is actually not a Proc or Method, and different value classes have been seen, then those would just be extra needless checks on the fast path.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks to the benchmarks! I have no more arguments - I pushed the version with respond_to? only

Given it's so simple after that, I would skip the helper and have the logic inline raise unless ...

I did it; however, in MRI we have 5 places with this check and maybe we'll return this later; I guess additional method is not affect anything, just readability

@ssnickolay ssnickolay force-pushed the feat/2.7-raise-error-for-noncallable-composition branch from 0fccd43 to 1d9409d Compare November 19, 2020 17:27
Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Thanks!

@eregon eregon added the in-ci The PR is being tested in CI. Do not push new commits. label Nov 19, 2020
@graalvmbot graalvmbot merged commit 7919f63 into oracle:master Nov 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-ci The PR is being tested in CI. Do not push new commits. oca-signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants