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

Change prepend MemoWise to extend MemoWise #253

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

jemmaissroff
Copy link
Contributor

@jemmaissroff jemmaissroff commented Dec 15, 2021

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:

  • 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

@codecov
Copy link

codecov bot commented Dec 15, 2021

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.

@jemmaissroff jemmaissroff force-pushed the uk-ji-refactor-squashed branch from f80b76e to 881beaa Compare December 15, 2021 15:11
@jemmaissroff jemmaissroff marked this pull request as ready for review December 15, 2021 23:55
@jemmaissroff jemmaissroff force-pushed the uk-ji-refactor-squashed branch from e7b1a47 to 81b2ff0 Compare January 4, 2022 15:29
@jemmaissroff jemmaissroff force-pushed the uk-ji-refactor-squashed branch from 81b2ff0 to 4057920 Compare February 1, 2022 19:28
@jemmaissroff jemmaissroff force-pushed the uk-ji-refactor-squashed branch from 4057920 to a5b4743 Compare April 1, 2022 09:58
@dhnaranjo
Copy link

dhnaranjo commented Sep 9, 2022

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?

@JacobEvelyn
Copy link
Member

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.

@ms-ati ms-ati mentioned this pull request Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants