matthewrudy / memoist

ActiveSupport::Memoizable with a few enhancements
MIT License
920 stars 99 forks source link

v0.13 changed #unmemoize_all behavior with subclasses #37

Closed bbxiao1 closed 8 years ago

bbxiao1 commented 8 years ago

The performance changes in v0.13 look really nice, but it seems to have caused a regression issue in the Rails app I am working on.

Here's a contrived example:

class Fruit
  extend Memoist
  attr_accessor :name, :calories

  def initialize(name, calories)
    @name = name
    @calories = calories
  end

  def generic_description
    "#{name}: #{calories}"
  end
  memoize :generic_description
end

class Apple < Fruit
  def initialize
    @name = "apple"
    @calories = 50
  end

  def specific_description
    "An #{name} a day keeps the doctor away!"
  end
  memoize :specific_description
end

IRB output:

apple = Apple.new #=> #<Apple:0x007fb77502b960 @name="apple", @calories=50>
apple.generic_description == "apple: 50" #=> true
apple.specific_description == "An apple a day keeps the doctor away!" #=> true
apple.name = "Granny smith apple" #=> "Granny smith apple"
apple.unmemoize_all #=> #<Set: {:specific_description}>
apple.generic_description == "apple: 50" #=> true
apple.specific_description == "An Granny smith apple a day keeps the doctor away!" #=> true

The main issue I want to highlight is the set of methods returned after apple.unmemoize_all. If I do the same example with v0.12:

apple = Apple.new #=> #<Apple:0x007fb51bcd0640 @name="apple", @calories=50>
apple.generic_description == "apple: 50" #=> true
apple.specific_description == "An apple a day keeps the doctor away!" #=> true
apple.name = "Granny smith apple" #=> "Granny smith apple"
apple.unmemoize_all #=> ["specific_description", "generic_description"]
apple.generic_description == "apple: 50" #=> false
apple.specific_description == "An Granny smith apple a day keeps the doctor away!" #=> true

Here, apple.unmemoize_all returns the full list.

If I am seeing the issue correctly, it is because the memoized method list is stored at the class level

Currently, I have reverted back to v0.12 just to get my build green. I believe it is possible to work around this issue by just passing in the methods manually with flush_cache. but it is not ideal.

matthewrudy commented 8 years ago

Hey @bbxiao1 that's a problem. Do you think this commit fixes it for you? https://github.com/jrafanie/memoist/commit/5e6f8fb2a7f0f059f9e6d7431410984e9bbb2151

bbxiao1 commented 8 years ago

Sorry for the delay. This kept getting bumped.

It looks correct and should fix it. My bundler-git-fu is failing and I am not sure if I can checkout that specific commit anymore (the v0.13 tag got pulled?).

matthewrudy commented 8 years ago

@bbxiao1 no problem. Just released 0.14.0 which should solve your problem.

Cheers

bbxiao1 commented 8 years ago

:confetti_ball: Looks good on my end!