Skip to content

Commit

Permalink
Implement identity map!
Browse files Browse the repository at this point in the history
  • Loading branch information
sferik committed Jun 2, 2012
1 parent 9e6823b commit 218479f
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 1 deletion.
12 changes: 11 additions & 1 deletion lib/twitter/base.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
require 'twitter/identity_map'

module Twitter
class Base
attr_accessor :attrs
alias :to_hash :attrs

@@identity_map = IdentityMap.new

This comment has been minimized.

Copy link
@jnunemaker

jnunemaker Jun 2, 2012

Collaborator

This isn't thread safe, right? I would think we'd need Thread.current[:twitter_identity_map] ||= IdentityMap.new or something. I'm no expert on threads though.

This comment has been minimized.

Copy link
@sferik

sferik Jun 2, 2012

Author Owner

Is your concern that this initialization would happen multiple times? I could make IdentityMap a singleton and call instance instead of new. Even if it is initialized multiple times, it wouldn't really cause anything bad to happen.


# Define methods that retrieve the value from an initialized instance variable Hash, using the attribute as a key
#
# @overload self.lazy_attr_reader(attr)
Expand All @@ -19,12 +23,18 @@ def self.lazy_attr_reader(*attrs)
end
end

def self.new(attrs={})
@@identity_map[self.name] ||= {}
@@identity_map[self.name][Marshal.dump(attrs)] || super(attrs)
end

# Initializes a new Base object
#
# @param attrs [Hash]
# @return [Twitter::Base]
def initialize(attrs={})
@attrs = attrs.dup
@attrs = attrs
@@identity_map[self.class.name][Marshal.dump(attrs)] = self
end

# Initializes a new Base object
Expand Down
8 changes: 8 additions & 0 deletions lib/twitter/identity_map.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
module Twitter

# Tracks objects to help ensure that each object gets loaded only once.
# See: http://www.martinfowler.com/eaaCatalog/identityMap.html
class IdentityMap < Hash
end

end
6 changes: 6 additions & 0 deletions spec/twitter/base_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,10 @@
end
end

describe "identical objects" do
it "should have the same object_id" do
@base.object_id.should == Twitter::Base.new('id' => 1).object_id
end
end

end

2 comments on commit 218479f

@jnunemaker
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One problem with identity maps is ensuring they are cleared. If someone used this in a web request, it would basically cache the lookup forever. Same problem with a single background process.

What rails and other things do is turn it off by default, then have middleware that turns it on for a request, ensuring things are cleared at the end. You can see what I did for toystore:

https://github.com/jnunemaker/toystore/blob/master/examples/identity_map.rb
https://github.com/jnunemaker/toystore/blob/master/lib/toy/identity_map.rb
https://github.com/jnunemaker/toystore/blob/master/spec/toy/identity_map_spec.rb
https://github.com/jnunemaker/toystore/blob/master/lib/toy/middleware/identity_map.rb
https://github.com/jnunemaker/toystore/blob/master/spec/toy/middleware/identity_map_spec.rb

@sferik
Copy link
Owner Author

@sferik sferik commented on 218479f Jun 2, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure you're reading the code correctly. HTTP requests are never cached and objects are only cached based on their values. If a request returns an object with even a single different value, it will be a cache miss, so data should never be stale. For objects that have ids (e.g. Twitter::Status or Twitter::User), we could check to see whether an object with the same id already exists in the cache and then perform an update instead of allocating a new object but it's not obvious to me whether that would be more or less efficient than doing it this way.

Please sign in to comment.