-
Notifications
You must be signed in to change notification settings - Fork 21.6k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Added Memoizable mixin for caching simple lazy loaded attributes
- Loading branch information
Showing
4 changed files
with
80 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
module ActiveSupport | ||
module Memoizable | ||
def self.included(base) #:nodoc: | ||
base.extend(ClassMethods) | ||
end | ||
|
||
module ClassMethods | ||
def memorize(symbol) | ||
original_method = "_unmemorized_#{symbol}" | ||
alias_method original_method, symbol | ||
class_eval <<-EOS, __FILE__, __LINE__ | ||
def #{symbol} | ||
if instance_variable_defined?(:@#{symbol}) | ||
@#{symbol} | ||
else | ||
@#{symbol} = #{original_method} | ||
end | ||
end | ||
EOS | ||
end | ||
end | ||
|
||
def freeze | ||
methods.each do |method| | ||
if m = method.to_s.match(/^_unmemorized_(.*)/) | ||
send(m[1]).freeze | ||
end | ||
end | ||
super | ||
end | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
require 'abstract_unit' | ||
|
||
uses_mocha 'Memoizable' do | ||
class MemoizableTest < Test::Unit::TestCase | ||
class Person | ||
include ActiveSupport::Memoizable | ||
|
||
def name | ||
fetch_name_from_floppy | ||
end | ||
memorize :name | ||
|
||
def age | ||
nil | ||
end | ||
memorize :age | ||
|
||
private | ||
def fetch_name_from_floppy | ||
"Josh" | ||
end | ||
end | ||
|
||
def test_memoization | ||
person = Person.new | ||
assert_equal "Josh", person.name | ||
|
||
person.expects(:fetch_name_from_floppy).never | ||
2.times { assert_equal "Josh", person.name } | ||
end | ||
|
||
def test_memoized_methods_are_frozen | ||
person = Person.new | ||
person.freeze | ||
assert_equal "Josh", person.name | ||
assert_equal true, person.name.frozen? | ||
end | ||
|
||
def test_memoization_frozen_with_nil_value | ||
person = Person.new | ||
person.freeze | ||
assert_equal nil, person.age | ||
end | ||
end | ||
end |
8a9934a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the inconsistency between memoize (the usage I am used to and used in the file name and the module name) and memorize (the actual call) intentional?
8a9934a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was fixed in 001c8beb4
8a9934a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why doesn’t it use alias_method_chain?
or atleast the same structure of aliasing.
with this we get a non-standard name for the function that bypasses memoizing.
I’d also want a .blah(true) which would force reload
8a9934a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have previously implemented the memoize feature as method_cache plugin at http://github.com/humanzz/method_cache. But method_cache had a couple more features: in addition to caching results in instance variables it also stored the result in the cache store. It’d be great if memoize adds such feature.
8a9934a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matthewruby I’m trying to avoid alias_method_chain. But if access to the original method is important, maybe we could name it “name_without_memoization”.
@humanzz No to RAILS_CACHE. Most of time you a memorizing simple string manipulations or integer calculations. They are either short lived and should die with the object or long live and you want to deal with the memcache protocol latency.
8a9934a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very valuable addition, definitely needed to go in as a separate module while patches that fix 500 problem with Nginx proxying get dusty.
8a9934a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and I like the way alias method chain is avoided here.
8a9934a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and I like the way alias method chain is avoided here.
8a9934a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, this is module is NOT include anywhere by default. To use it include ActiveSupport::Memoizable
8a9934a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as implementations go, this is alright, but there doesn’t appear to be a simple way to reload the data. Personally, I like Marcel Molina’s implementation of this same feature in the AWS-S3 library.
8a9934a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn’t realize that amount of people who would be using this outside of core. I added a few of your requests.
http://github.com/rails/rails/commit/e1f23da53cef20a60e4bf458d959fe2bfe7d52ea