Rails Ruby on Rails → 3.x

Comments and changes to this ticket

• You flagged this item as spam.
  

  Glenn Powell May 5th, 2009 @ 03:09 PM

  Is anyone else experiencing this behavior?

• You flagged this item as spam.
  

  Kurt Stephens May 7th, 2009 @ 06:10 AM

  We've used patterns like the following to allow caching of computed nil and false results:

  def cached_value

  (@cached_value ||= [ computed_value ]).first
  
  

  end def computed_value

  something.that.might.return.false
  
  

  end

  Does memoize use false (and/or nil) as the "uncached" sentinel value? If it does, it's very broken.

• You flagged this item as spam.
  

  Kurt Stephens May 7th, 2009 @ 06:11 AM

  Gah.. that should have been formatted as:

  def cached_value
    (@cached_value ||= [ computed_value ]).first
  end
  def computed_value
    something.that.might.return.false
  end
  

• You flagged this item as spam.
  

  Kurt Stephens May 7th, 2009 @ 06:26 AM

  Here's fix for activesupport/lib/active_support/memoizable.rb:

  module ActiveSupport
    module SafelyMemoizable
      def safely_memoize(*symbols)
        symbols.each do |symbol|
          class_eval <<-RUBY, __FILE__, __LINE__ + 1
            def #{symbol}(*args)
              memoized = @_memoized_#{symbol} || ::ActiveSupport::ConcurrentHash.new
              (memoized[args] ||= [ memoized_#{symbol}(*args) ]).first
            end
          RUBY
        end
      end
    end
  
  

• You flagged this item as spam.
  

  Kurt Stephens May 7th, 2009 @ 06:27 AM

  Also, should:

  memoized = @_memoized_#{symbol} || ::ActiveSupport::ConcurrentHash.new
  

  be:

  memoized = (@_memoized_#{symbol} ||= ::ActiveSupport::ConcurrentHash.new)
  

  ???

• You flagged this item as spam.
  

  Kurt Stephens May 7th, 2009 @ 06:29 AM

  There also a problem if method being memoized is named "memoized". :) The "memoized" local variable in the code template should be gensym'ed.

• You flagged this item as spam.
  

  Kurt Stephens May 7th, 2009 @ 06:34 AM

  The elements in args are not protected from side-effects either so something like:

    a = [ 1, 2 ]
    puts memoized_method(a)
    a[0] = 5
    puts memoized_method(a)
  

  will likely have undetermined effects. Cache keys must be immutable, perhaps use:

    memoized[Marshal.dump(args)] ||= ...
  

  at a great performance cost? It's a tough problem to solve.

  Caveat Emptor...

• You flagged this item as spam.
  

  Glenn Powell May 7th, 2009 @ 07:12 AM

  Well, actually I don't think my problem has to do with the result of the memoized method necessary, but more with the fact that the value of the default parameter is 'false'. It seems to be ignoring if I pass in 'true' as this parameter. It will always just default it to false. Here's a better example that doesn't use a false return value:

  
  def some_method(param1, param2 = false)
    return param2 ? param1 : "FALSE"
  end
  memoize :some_method
  

  Then when you call the method:

  
  >> some_method("TRUE", false)
  => "FALSE"
  
  >> some_method("TRUE", true)
  => "FALSE"
  

  You get "FALSE" both times, when the second time should have resulted in "TRUE". Does this make more sense?

• You flagged this item as spam.
  

  Kurt Stephens May 7th, 2009 @ 08:05 AM

  This is why:

              def #{symbol}(*args)                                                   #   def mime_type(*args)
                #{memoized_ivar} ||= {} unless frozen?                               #     @_memoized_mime_type ||= {} unless frozen?
                reload = args.pop if args.last == true || args.last == :reload       #     reload = args.pop if args.last == true || args.last == :reload
                                                                                     #
  

  The memoized wrapper assumes that a trailing true or :reload means "reload". Having the memoized wrapper try to handle a "meta argument" is a bad idea.

  There should probably be a separate method to handle reload requests.

  Not sure why methods of different aritys have different caching code.

• You flagged this item as spam.
  

  Kurt Stephens May 7th, 2009 @ 08:32 AM

  To fix this for your calling sequence would require removal of the "reload" magic. IMO, the reload magic is a really bad API idea, but it would require everyone else to stop using it.

• You flagged this item as spam.
  

  Glenn Powell May 7th, 2009 @ 01:58 PM

  Kurt, Thanks for the explanation. I'll try to work around it for now.

• 

  Jeremy Kemper May 4th, 2010 @ 06:48 PM

  • Milestone changed from “2.x” to “3.x”

  [bulk edit]

• 

  Santiago Pastorino February 2nd, 2011 @ 05:03 PM

  • State changed from “new” to “open”
  • Tag changed from “2.3.2, 2.3.2.1, 2.3.x, memoize” to “232, 2321, 23x, memoize”
  • Importance changed from “” to “”

  This issue has been automatically marked as stale because it has not been commented on for at least three months.

  The resources of the Rails core team are limited, and so we are asking for your help. If you can still reproduce this error on the 3-0-stable branch or on master, please reply with all of the information you have about it and add "[state:open]" to your comment. This will reopen the ticket for review. Likewise, if you feel that this is a very important feature for Rails to include, please reply with your explanation so we can consider it.

  Thank you for all your contributions, and we hope you will understand this step to focus our efforts where they are most helpful.

• 

  Santiago Pastorino February 2nd, 2011 @ 05:03 PM

  • State changed from “open” to “stale”