memo_wise icon indicating copy to clipboard operation
memo_wise copied to clipboard

Change `prepend MemoWise` to `extend MemoWise`

Open jemmaissroff opened this issue 3 years ago • 3 comments

Co-authored-by: Jemma Issroff [email protected]

What the refactor does:

The only public facing change in this refactor is that instead of prepend MemoWise, clients will now extend MemoWise. Internally, this refactor simplifies the machinery necessary to get memoization working, including eliminating back and forth of singleton classes and deferring definitions of preset_memo_wise and reset_memo_wise. With this change, MemoWise will also no longer redefine methods, simplifying delegation to the original method and making it easier to determine which methods are memoized.

Guiding principles for the refactor:

  • Mixing in MemoWise module should be as nonintrusive as possible. By utilizing extend, we make sure that mixing in MemoWise only adds the memo_wise method on the the class, which is the main API of the library.
  • In the same spirit, currently the library overrides a lot of methods (including initialize) and eagerly defines many instance variables on the mixin site, thus unnecessarily polluting the target.
  • Internal implementation, in the current form, is more complex that it could be. For example, the internal api has to resort to walking the ObjectSpace to find attached objects to singleton classes, or the library needs to keep checking if the mixin site is a singleton class or not.
  • The library is essentially doing an "alias-method-chain" dance which is brittle and makes delegating arguments to the actual method needlessly complex. Instead, prepending a module on the class which defines the same methods allows us to just use super to delegate to the actual instance. Moreover, this method of prepended methods plays nicer with other forms of method wrapping that other libraries could be using as well (for example, in its current form MemoWise would not play nicely with Sorbet runtime).

How the refactor works:

Step 1. Call extend MemoWise from a class/module.

This exposes the method memo_wise to the class/module as a class method.

Step 2. Call the memo_wise method on an instance method (for example, memo_wise :example_method).

With the first memo_wise method call, a module named MemoWiseMethods is created. This module has preset_memo_wise, reset_memo_wise and freeze as instance methods. The memo_wise call also adds a memoized shadow instance method on this module (for example, MemoWiseMethods#example_method).

Step 3. The MemoWiseMethods module is automatically prepended on the original class/module.

The use of prepend means that the shadow methods on this module take precedence over the original methods. Calling super from within the shadow methods is sufficient for accessing the original methods (instead of doing the "alias-method-chain" dance).

Notes:

  • Calling memo_wise on class methods makes the singleton class of the class/module extend MemoWise, and calls memo_wise for the instance methods on the singleton class. Accordingly, preset_memo_wise and reset_memo_wise are only defined as class methods the first time memo_wise is called on a class method (with self:).
  • Frozen objects are supported by shadowing the freeze method and initializing internal state before calling super.

image

Before merging:

  • [x] Copy the table printed at the end of the latest benchmark results into the README.md and update this PR
  • [ ] If this change merits an update to CHANGELOG.md, add an entry following Keep a Changelog guidelines with semantic versioning

jemmaissroff avatar Dec 15 '21 14:12 jemmaissroff

Codecov Report

Merging #253 (cb34c22) into main (8209474) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #253   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         3    +1     
  Lines          166       144   -22     
  Branches        83        67   -16     
=========================================
- Hits           166       144   -22     
Impacted Files Coverage Δ
lib/memo_wise.rb 100.00% <100.00%> (ø)
lib/memo_wise/internal_api.rb 100.00% <100.00%> (ø)
lib/memo_wise/module_builder.rb 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8209474...cb34c22. Read the comment docs.

codecov[bot] avatar Dec 15 '21 14:12 codecov[bot]

So what's up with this here PR? I'm looking to get working with Sorbet in my app and it's straight incompatible with prepend, which is a bummer. The branch has no problem with my tests, but if y'all abandoned this effort for whatever reason I'd rather not depend on it, you know?

dhnaranjo avatar Sep 09 '22 17:09 dhnaranjo

Hi @dhnaranjo @halo! Sorry I missed the question above! We had paused this work when we found it wasn't as performant in our benchmarks and we were unable to figure out why. I'm in the process of upgrading our benchmarks to Ruby 3.2 and will take a look at how this branch performs under 3.2 once that's done!

While I don't use Sorbet, I do want this gem to be usable by folks who do.

JacobEvelyn avatar Feb 13 '23 15:02 JacobEvelyn