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

Global cache flush #25

Open
smazhara opened this issue Aug 21, 2014 · 10 comments
Open

Global cache flush #25

smazhara opened this issue Aug 21, 2014 · 10 comments

Comments

@smazhara
Copy link

Is there a way to globally flush the cache?

I have some nasty order dependent bugs in my rspecs where objects with their caches seem to be leaking from one spec to another.

So far, the best workaround I came up with is this

ObjectSpace.each_object { |o| o.flush_cache if o.respond_to? :flush_cache }

horrible workaround in my spec/spec_helper.rb.

Can we do better?

@elado
Copy link

elado commented Apr 17, 2015

+1

@elado
Copy link

elado commented Apr 17, 2015

I did this for now, seems faster but doesn't support instances (which is OK for specs, I guess):

require 'binding_of_caller'
require 'memoist'

module Memoist
  @@_classes = Set.new

  alias old_memoize memoize

  def memoize(*method_names)
    @@_classes << if self.singleton_class?
      binding.callers[2].receiver
    else
      self
    end

    old_memoize(*method_names)
  end

  def self.flush_cache
    @@_classes.each { |x| x.try(:flush_cache) }
  end
end


# spec_helper

  config.after(:each) do
    Memoist.flush_cache
  end

@jrafanie
Copy link
Contributor

Can you do

ObjectSpace.each_object(Memoist) { |o| o.flush_cache }

I believe each class that extends memoist should be handled by that... It should be faster

@matthewrudy
Copy link
Owner

This is interesting.

You don't want to retain a list of all memoised objects, or else they can't be garbage collected.

But you could selectively add objects to a list
And flush the cache.

Something like

memoize :foo, track: true

But Id say, if you`re memoizing a whole bunch of global objects,
You're probably doing something wrong.

If you handcoded your class

class Foo
  def self.bar
    @bar ||= something_you_want_to_memoizr
  end
end

How would you flush that?

@JoeMcB
Copy link
Contributor

JoeMcB commented Apr 5, 2019

Bumping a 5 year old issue! 💯

We're not having issues with memoized instances leaking between specs, but we've hit a nasty bit of dependency using class-level memoization.

Here's our solution (currently sits in our spec code but can be extracted):

      memoized_classes = ObjectSpace.each_object(Class).select do |obj|
          obj.included_modules.include? Memoist::InstanceMethods
      end

      memoized_classes.each do |mobj|
        mobj.all_memoized_structs.each do |struct|
          mobj.remove_instance_variable(struct.ivar) if mobj.instance_variable_defined?(struct.ivar)
        end
      end

Class memoization occurs at the singleton class level, but flush_cache only exists at class level. I haven't found a reliable way to grab the class (as opposed to singleton class) from ObjectSpace. As a result, we need to duplicate some of the logic from within Memoist to clear out the ivars.

This approach doesn't introduce the GC issues of tracking every memoized object, but constantly scanning ObjectSpace for classes isn't the most efficient approach either... you need to check after each spec if you're you're not preloading the entire application though.

@matthewrudy
Copy link
Owner

@JoeMcB what's a minimal example of the problem?

eg. the tests memoize the Book class, and flush_cache works fine?

def test_class_flush_cache
@book.memoize_all
assert_equal "My Life by Brian 'Fudge' Turmuck", @book.full_title
Book.memoize_all
assert_instance_of Array, Book.instance_variable_get(:@_memoized_all_types)
Book.flush_cache
assert_equal false, Book.instance_variable_defined?(:@_memoized_all_types)
end

@jrafanie
Copy link
Contributor

jrafanie commented Apr 8, 2019

We're not having issues with memoized instances leaking between specs, but we've hit a nasty bit of dependency using class-level memoization.

Here's our solution (currently sits in our spec code but can be extracted):

@JoeMcB in addition to the minimal example, can you describe the problem? I'm not sure what you mean by nasty bit of dependency using class-level memoization What is your solution solving? Test contamination?

@JoeMcB
Copy link
Contributor

JoeMcB commented Apr 8, 2019

Yep, that works fine! I actually submitted the PR for that feature. :)

Our issue isn't calling flush on any memoized Class, it's needing to call flush on every memoized class. In our specific case, we're memoizing quite a few objects (at a class level) through our application. We've been adding a line ClassName.flush_cache to a before hook for our test suite in order to make sure memoized data isn't persisting between tests.

config.before(:each) do
    Design::OptionAttributeCache.flush_cache
    Product::SubclassPriceService.flush_cache
    # and so on
end

We've seen more than a few instances of a developer forgetting to add their newly memoized class into that spec configuration though. It'd be considerably easier from a testing perspective to have a Memoist.flush_all method that flushes every memoized class at once to future proof our specs against additional uses of Memoist.

The code posted above is working for us. I can wrap that in a PR if it'd be helpful for the base library.

@jrafanie
Copy link
Contributor

jrafanie commented Apr 8, 2019

@JoeMcB be careful though, perhaps the code isn't clearing the cache when it should be. Maybe the tests are trying to demonstrate this?

We have one mixin we use memoist for with many methods in ManageIQ, in our relationship mixin, which is included in many of our classes. We call flush_cache in one place, the clear_relationship_cache here.

If you notice, we call clear_relationship_cache whenever we're making changes to a relationship so it's always unset and clean so the new relationship can be memoized.

@jrafanie
Copy link
Contributor

jrafanie commented Apr 8, 2019

Sorry, that was a super complicated example but the point is to have one entrypoint to the memoization so you can easily clear the caching when you need to store a new value.

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

No branches or pull requests

5 participants