always set memo_wise hash if not set for some reason

[joevandyk]

Fixes #302 for us

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

[JacobEvelyn]

Interesting, thanks @joevandyk ! Would you be able to write a failing test that passes with this change? If you need help navigating our test files let me know and I can try to add one based on your previous example.

[joevandyk]

@JacobEvelyn The tests probably aren't formatted exactly as they should be, but I think it illustrates the problem.

[joevandyk]

@JacobEvelyn any thoughts on this one?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9b289a9) 100.00% compared to head (fae1c66) 100.00%.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #321   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          179       180    +1     
  Branches        88        88           
=========================================
+ Hits           179       180    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[JacobEvelyn]

Hey @joevandyk ! Thanks so much for the failing tests, and so sorry to have taken so long to get back to you.

I agree this is a problem that should be fixed. I don't love how the ||= approach introduces a second way the instance variable can get initialized. I'd rather we just have one path for that for simplicity. I also am a bit saddened that this adds a performance hit (esp. in Ruby 2.7) for these memoized method calls—the benchmark numbers from the GitHub CI run with this change look like this (numbers below 1.00x indicate a slowdown compared to the main branch):

Method arguments	MemoWise_GitHubMain (1.8.0)
() (none)	0.75x
(a)	0.80x
(a, b)	0.83x
(a:)	0.81x
(a:, b:)	0.82x
(a, b:)	0.82x
(a, *args)	0.96x
(a:, **kwargs)	1.00x
(a, *args, b:, **kwargs)	0.98x

Is there a way to detect this case better and initialize the instance variable then?

[joevandyk]

Thanks for looking at this!

To be honest, I don't know why the instance variable isn't always initialized.

This fix is a bandaid for what I assume is properly initializing the variable, unfortunately my Ruby's not good enough to understand this gem at that deep a level. :)

[JacobEvelyn]

Okay I dug in a bit more here. The challenge is that even if we change our def memo_wise to have something like this:

def klass.included(base)
  base.class_eval <<~HEREDOC, __FILE__, __LINE__ + 1
    def initialize(#{ALL_ARGS})
      MemoWise::InternalAPI.create_memo_wise_state!(self)
      super
    end
  HEREDOC
end

this only works if the class we care about defines initialize before includeing the module:

class MyClass
  def initialize(*) ; end
  include MyModule
end

This is because the included method gets called immediately and the initialize method it adds will get overridden by the one in MyClass if the include happens first.

It's not ideal, but I wonder if a solution for your use case is to just use prepend for your module instead of include?

There's also some other work going on right now in #324 and #326 to fix some similar issues, and I'm trying to see if a similar solution would work here.

[ms-ati]

What if we change MemoWise to actually error out if we use include instead of prepend?

[JacobEvelyn]

What if we change MemoWise to actually error out if we use include instead of prepend?

@ms-ati I think the issue is it's not caused by someone includeing MemoWise, they're includeing their own module which prepends MemoWise.

@joevandyk I'm no longer confident I can get something working on the code to resolve this issue for you. Would one of these workarounds work for you instead?

• change your include ModuleMethods to prepend ModuleMethods
• move your include ModuleMethods to be below your def initialize?

[ms-ati]

QQ: What would happen if everything stayed the same (that is: include ModuleMethods at the top, before the def initialize), but in initialize it calls super either at beginning or end?

Does that still trigger the MemoWise initialize?

[ms-ati]

@JacobEvelyn I am wondering if we should consider making use of Module#method_added as a callback, and if the method added is #initialize, take some action?

[sunny]

I’ve had issues with memo_wise and ActiveRecord’s latest marshalling format.

ActiveRecord.marshalling_format_version = 7.1

class User < ActiveRecord::Base
  prepend MemoWise
end

Rails.cache.fetch("test") { User.new }.instance_variable_get(:@_memo_wise)
# => {}
Rails.cache.fetch("test") { User.new }.instance_variable_get(:@_memo_wise)
# => nil

This branch fixes the issue 🙏🏻