Actually use fast path for alias_method when checked #3339
Conversation
| original_visibility = visibility_method_name(mod, method_sig.method_name) | ||
| # Use `original_method.name` not `method_sig.method_name` to get visibility | ||
| # so that we handle aliases correctly (we want the visibility before aliasing). | ||
| original_visibility = visibility_method_name(mod, original_method.name) |
There was a problem hiding this comment.
Just checking my understanding: was this previously getting the visibility for the original method, instead of the visibility of the alias? So for this example:
class A
def foo
puts "hi"
end
alias :bar :foo
private :bar
def test
bar
end
end
A.new.testWould we have used public as the visibility for the :bar alias, instead of private?
There was a problem hiding this comment.
This was previously getting the visibility of the original method, and continues to be getting the visibility of the original method. It probably should be getting the visibility of the alias, but we'd need to thread the aliasing module through, since that won't necessarily be the same as the mod here which is the original module.
But now that I'm looking at this I'm getting confused, I don't know how this is working at all with mod as the original module...
There was a problem hiding this comment.
Do we already have a test case for aliases where the modules differ?
There was a problem hiding this comment.
We do, but we weren't asserting that we got the fast path there. Now we are (and I fixed the bug, see other comment)
| # Should use fast path | ||
| obj = klass.new | ||
| allocs = counting_allocations {obj.bar} | ||
| assert(allocs < 5) |
There was a problem hiding this comment.
Do you have a bound for the number of allocations done on the slow path? Mostly just curious to know what the difference between the two is.
There was a problem hiding this comment.
It's 20-30 on the slow path (varies a bit by exact case), 3 on the fast path.
| @@ -356,8 +349,8 @@ def self.build_sig(hook_mod, method_name, original_method, current_declaration, | |||
| end | |||
| end | |||
|
|
|||
| def self.unwrap_method(hook_mod, signature, original_method) | |||
There was a problem hiding this comment.
Note: hook_mod was dead code here, it wasn't being used
| @@ -304,7 +297,7 @@ def self.run_sig(hook_mod, method_name, original_method, declaration_block) | |||
| Signature.new_untyped(method: original_method) | |||
| end | |||
|
|
|||
| unwrap_method(hook_mod, signature, original_method) | |||
| unwrap_method(signature.method.owner, signature, original_method) | |||
There was a problem hiding this comment.
This ensures that in the non-alias case, we don't change behavior, since the thing we pass as mod will now actually be used
When attempting to self-modify-away the initial slow wrapper for a method created via
aliasoralias_method, and calling intounwrap_method, pass a Signature object which has the alias as themethod_name, instead of the original method name.Before this PR, the
unwrap_methodcall was actually just replacing the original method each time, and never affecting the alias. This worked but was unintentional.With this PR,
unwrap_methodwill actually do the right thing here for both checked and unchecked methods, so we can simplify.Motivation
Followup on #3317 which had the desired effect of allowing aliases to use the fast path for unchecked, but not for checked code.
#3317 has been much more of a journey than I expected - sorry for the churn! - but I do think we're ending up in a better place.
Test plan
Added test assertions & also tested with pay-server, where I saw the expected decrease in allocations.