Simpler and faster check for the delegation fastpath #39
Conversation
|
|
||
| method_call = ".__send__(:#{method}, *args, &block)" | ||
| if _valid_method?(method) | ||
| if method.match?(/\A[_a-zA-Z]\w*[?!]?\z/) |
There was a problem hiding this comment.
But this will fail if the method has any Unicode characters in its name, no?
There was a problem hiding this comment.
By the way, if this is trying to check if the method name is a valid identifier then Prism has methods for doing that check out of the box. Maybe we should use those
There was a problem hiding this comment.
It's checking for two things, first that it's a valid name, but also that it can be called with ..
e.g. foo= is valid, but _.foo = *args, &block isn't.
But this will fail if the method has any Unicode characters in its name, no?
Yes, but the idea is to be conservative, the regexp can be improved (or we could use prism) if we want, but as long as 99.9% of methods go in the fast path, it's fine IMO.
byroot
force-pushed
the
avoid-syntax-errors
branch
from
508eed6 to
6a2eff9
Compare
|
You deleted |
Fix: ruby#35 [Bug #21708] Trying to compile code to check if a method can use the delegation fastpath is a bit wasteful and cause `RUPYOPT=-d` to be full of misleading errors. It's simpler and faster to use a simple regexp to do the same check.
byroot
force-pushed
the
avoid-syntax-errors
branch
from
6a2eff9 to
de1fbd1
Compare
|
@Earlopain good catch, fixed now. |
jeremyevans
left a comment
jeremyevans
left a comment
There was a problem hiding this comment.
I like the approach, I think there is just one issue with it.
| end | ||
| end; | ||
| method_call = <<~RUBY.chomp | ||
| if defined?(_.#{method}) |
There was a problem hiding this comment.
Before Ruby 3, using defined? for methods does not work correctly if the method has been refined but the refinement is not activated. See https://bugs.ruby-lang.org/issues/16932 . I found this out the hard way in Sequel: jeremyevans/sequel#1919 .
In this case, defined? would return truthy if the method had an unactivated refinement. If the method being refined was private and the refinement method is public, I believe this would cause the fast path to be used in a case where it would fail.
Example:
module M
refine Object do
public :puts
end
end
o = Object.new
o.puts '' if defined?(o.puts)On Ruby < 3, we could probably use something like Kernel === _ && _.respond_to?(:#{method}) . Or we could bump the required_ruby_version to 3.0.
There was a problem hiding this comment.
I'm fine with making that change, but note that I only moved the defined? so it's a pre-existing bug, probably better to handle it in a dedicated PR?
There was a problem hiding this comment.
Sure, that makes sense.
|
This seems to have broken Ruby master: I'll look into why. |
|
So |
|
World this be a ZJIT bug? Seems like the ruby-bench CI only runs with ZJIT? |
@k0kubun said it expect |
|
It's a ZJIT bug. I excluded the failed benchmarks from the ZJIT CI for now ruby/ruby#15479, and also added a non-ZJIT ruby-bench job to make it easier to understand whether it's about ZJIT or not ruby/ruby#15480. We'll work on fixing ZJIT to un-exclude the benchmarks. |
6 participants
Fix: #35
[Bug #21708]
Trying to compile code to check if a method can use the delegation fastpath is a bit wasteful and cause
RUPYOPT=-dto be full of misleading errors.It's simpler and faster to use a simple regexp to do the same check.