Decorate .instance_method to proxy original params #163
Conversation
Codecov Report
@@ Coverage Diff @@
## main #163 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 123 134 +11
=========================================
+ Hits 123 134 +11
Continue to review full report at Codecov.
|
ms-ati
force-pushed
the
proxy-method-params
branch
from
9d62f80
to
8032582
Compare
ms-ati
force-pushed
the
proxy-method-params
branch
from
b420fc4
to
3262e94
Compare
|
Re: #165 |
lib/memo_wise.rb
Outdated
|
|
||
| orig_method = | ||
| if symbol == :initialize | ||
| curr_method.super_method |
There was a problem hiding this comment.
The fact that this evaluates to super.super_method is kind of confusing to me. Would you mind adding some comments here for what this is doing and why we need to special-case it?
There was a problem hiding this comment.
I'll add explicit documentation of the three paths:
-
initialize- callsuperwhich is the original implementation ofModule#instance_method, get back theUnboundMethodwhich represents ourMemoWiseversion ofinitialize-- then ask it for#super_method, because it know the original method that it is overriding -
Any memo-wised method - we need to look up the original method by it's renamed symbol, and call
superwhich is the original implementation ofinstance_method, using that name -
Anything else - we have
orig_method = nil
There was a problem hiding this comment.
Modified per @jemmaissroff above and documented per this discussion, please re-review?
lib/memo_wise.rb
Outdated
| super(original_memo_wised_name) | ||
| end | ||
|
|
||
| unless orig_method.nil? |
There was a problem hiding this comment.
Could you add a comment explaining when orig_method will be nil? Is it just when there's no initialize method defined?
There was a problem hiding this comment.
I'll clarify the cases -- just returning curr_method when it was not a memo_wised method and not initialize -- can make it clearer
| @@ -404,7 +404,7 @@ def #{method_name}#{args_str} | |||
| end | |||
|
|
|||
| unless target.singleton_class? | |||
| # Create certain class methods to implement .preset_memo_wise | |||
| # Create class methods to implement .preset_memo_wise and .reset_memo_wise | |||
There was a problem hiding this comment.
Unrelated comment clarification
lib/memo_wise.rb
Outdated
| # to proxy the original `UnboundMethod#parameters` results. We want the | ||
| # parameters to reflect the original method in order to support callers | ||
| # who want to use Ruby reflection to process the method parameters, | ||
| # because the generated memoized method would have a generic set of |
There was a problem hiding this comment.
Would you mind tweaking this to say something like ... because our overridden #initialize method, and in some cases the generated memoized methods, would have ...? (I'm also fine with being more vague—just wanted to flag that it's not always memoized methods and the args aren't always generic.)
Not sure if you're aware but avoiding generic params like *args in many cases is how @jemmaissroff and I got some big speedups recently.
There was a problem hiding this comment.
Changed as requested!
In Panorama spec helper code 'initialized_double', we observed
that MemoWise interrupts the use of Ruby reflection on method
parameters, by replacing with delegating methods that have overly
generic signatures, like:
def initialize(...)
def slow_method(*arg, **kwargs, &block)
After investigation, we had determined that it was not worth
the complexity cost, and potential fragility, to pursue a solution
where we fully re-create the original method parameter signatures
in our delegating implementations.
However, we had decided to explore a more minimalistic alternative.
This PR is a minimalistic alternative, which narrowly overrides only
the implementation of {Module#instance_method} so that, in the case
of either #initialize or a method that has had .memo_wise called on it,
we look up the original method and proxy the original result value of
calling UnboundMethod#parameters on it.
ms-ati
force-pushed
the
proxy-method-params
branch
from
6f6676a
to
48ff701
Compare
In Panorama spec helper code 'initialized_double', we observed
that MemoWise interrupts the use of Ruby reflection on method
parameters, by replacing with delegating methods that have overly
generic signatures, like:
After investigation, we had determined that it was not worth
the complexity cost, and potential fragility, to pursue a solution
where we re-create the original method parmeter signatures for our
delegating implementations.
This PR is a minimalistic alternative, which narrowly overrides only
the implementation of {Module#instance_method} so that, in the case
of either #initialize or a method that has had .memo_wise called on it,
we look up the original method and proxy the original result value of
calling UnboundMethod#parameters on it.