Skip to content
This repository

Remove IdentityMap #5261

Merged
merged 4 commits into from about 2 years ago
Carlos Antonio da Silva

Identity Map was added in Rails 3.1 and due to some known issues with associations, it never graduated to be an "enabled-by-default" feature. Since we haven't seen any interest in solving such issues, it seems reasonable to remove the feature from Rails codebase.

More info: 302c912

Guillermo Iguaran
Owner

If someone wants save it is time to talk now :)

Jeremy Kemper
Owner
...due to some known issues with associations...

Do we have a list of issues?

In my app, it caused strange issues with transactions in tests, but I didn't track down the root cause. Disabling the identity map fixed it.

José Valim
Owner

@jeremy this commit documents the main association issue:

302c912

I also remember debugging some transaction bugs in my test environment but that was a long time ago and I don't remember the details. :(

José Valim
Owner

Maybe it would be worthy to add the explanation why we are removing the feature to the CHANGELOG as well?

Jeremy Kemper
Owner

Thanks @josevalim

Carlos Antonio da Silva

@josevalim @jeremy I'm going to add a bit of explanation and also point to the same commit in the CHANGELOG, and add an entry to the *Upgrading Rails" guide (I was doing that for docrails separately because last changes on this guide weren't merged on master).

Vijay Dev
Collaborator

@carlosantoniodasilva The docrails changes are merged into master. You can make the changes as part of this PR itself.

Carlos Antonio da Silva

@vijaydev thanks, I've added a section to the "upgrading rails" guide about this IdentityMap removal, and have expanded the changelog comments a bit.

Let me know if I there is something else to improve or docs to add. Thanks.

railties/guides/source/upgrading_ruby_on_rails.textile
((8 lines not shown))
37 41
 h3. Upgrading from Rails 3.1 to Rails 3.2
38 42
 
39 43
 If your application is currently on any version of Rails older than 3.1.x, you should upgrade to Rails 3.1 before attempting an update to Rails 3.2.
40 44
 
41  
-The following changes are meant for upgrading your application to Rails 3.2.1, the latest 3.2.x version of Rails.
3
Vijay Dev Collaborator
vijaydev added a note March 04, 2012

can you do this (and the line below) in a different commit? This needs to go into 3-2-stable as well and it will be easier to cherry pick from a different commit.

Sure, will split them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
railties/guides/source/upgrading_ruby_on_rails.textile
... ...
@@ -34,18 +34,22 @@ h4(#plugins4_0). vendor/plugins
34 34
 
35 35
 Rails 4.0 no longer supports loading plugins from <tt>vendor/plugins</tt>. You must replace any plugins by extracting them to gems and adding them to your Gemfile. If you choose not to make them gems, you can move them into, say, <tt>lib/my_plugin/*</tt> and add an appropriate initializer in <tt>config/initializers/my_plugin.rb</tt>.
36 36
 
  37
+h4(#identity_map4_0). IdentityMap
  38
+
  39
+Rails 4.0 has removed <tt>IdentityMap</tt> from <tt>ActiveRecord</tt>, due to some inconsistencies with associations. If you have manually enabled it in your application, you will have to remove the following config that has no effect anymore: <tt>config.active_record.identity_map</tt>.
2
Vijay Dev Collaborator
vijaydev added a note March 04, 2012

Can we have this 302c912 mentioned here too?

Done, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Keith Pitt

Is there a plan on moving it into a gem? We actually use it quite heavily for performance reasons in our app.

José Valim
Owner

@keithpitt there is no plan to make a gem out of this since it requires a considerable amount of hooks inside Active Record. Maybe you could try to address the main issue that is leading to Identity Map removal? The current state of Identity Map can lead to severe bugs in your application.

Ferdinand Svehla

Just watching

José Valim josevalim merged commit 7950622 into from March 13, 2012
José Valim josevalim closed this March 13, 2012
Arturo Pie

We also use Identity Map in our app. We just enable it in some parts of our code where we are sure we won't have the association issue. This has improved a lot the performance of our app, it would be nice if we can still use it in rails 4.

Carlos Antonio da Silva

hey @arturopie, there is a proposal fix for the related issue with Identity Map, #5442. I believe that if that issue is fixed, it should be reverted back to Rails. You can watch that feature and help if possible. Thanks.

Sean Kirby

I would like to note that the 'bug' that #5442 fixes is just how ActiveRecord works. It's possible that having the IdentityMap turned on would extend the scope of the issue, but still not beyond the context of a request. The problem is really how ActiveRecord caches the results of has_many's and belongs_to's without regard to what's going on in the DB.

The IdentityMap is a huge improvement to ActiveRecord's memory usage for more complicated actions. Especially those that need to iterate over a collection. The reduction in memory usage in our application is huge with response times improving 3x by just turning the IdentityMap on.

Sean Kirby

If the issue with the IdentityMap is how Post.destroy(post.id) behaves (from 302c912), could we not just fix that instead of removing the IdentityMap?

Carlos Antonio da Silva

@sskirby sure we can =), as I commented above, as soon as someone is able to fix the related IdentityMap issues, it'll be probably back into Rails. There is this proposal fix already #5442, please feel free to watch there and help if possible. Thanks.

Evgeniy Dolzhenko

I believe there are other issues with IM at the moment, consider this for example

ruby-1.9.3-p125 :002 > ActiveRecord::IdentityMap.use do
ruby-1.9.3-p125 :003 > u = User.select(:id).first
ruby-1.9.3-p125 :004?> u.name
ruby-1.9.3-p125 :005?> end
User Load (2.4ms) SELECT id FROM "users" LIMIT 1
ActiveModel::MissingAttributeError: missing attribute: name

We were also seeing harder to demonstrate bug with dirty tracking and BigDecimal value being returned as strings (will try to submit issue if the decision on keeping it changes)

Guillermo Iguaran
Owner

@keithpitt @arturopie @sskirby can you test the proposal fix posted in #5442 in your apps?

Sean Kirby

@carlosantoniodasilva So just fix the problem outlined at 302c912 and it's back in?

Don't you think that removing it is a little drastic? Especially when the benefits it provides are incredible?

@guilleiguaran can do.

Carlos Antonio da Silva

@sskirby hey, I don't think it's so drastic. Without removing it, no one would ever remember about the issue anymore, and Rails would be responsible for code that could break expectations easily and make some real harm. Now we can bring it back to discussion, and if possible - and hopefully - fix it :)

Arturo Pie

@carlosantoniodasilva, I see your point, but in my opinion, it'd have been better if the intention to remove IM (if the issues are not fixed) had been shared with the rails community ahead of time. If you read the comments in 302c912, it says: "This incosistency is meant to be fixed in future Rails releases." I bet many people are waiting for these fixes, and will be surprised when they find that this feature was removed.

Carlos Antonio da Silva

@arturopie sure, I agree, and I believe that's why the issue stayed here for around 10 days before getting merged. And yes, it was supposed to be fixed in future versions of Rails, but the problem has been around for some time already and no possible fix appeared up to now :). Anyway, it's not completely settled to be removed forever, we can always fix and revert the merge, so lets focus our work on that to get it back on master now that we see that people are using it and willing to fix :).

gamov

That's so sad... IdentityMap would be such a performance boost if working properly.
In 2012, it's so stupid that loading a model from two different side of an association doesn't return the same object.

Ryan Wallace

To deal with @dolzenko's issue, we could only save models to the identity map if all of their attributes were retrieved (any downsides to this?). Here is another gist with a small monkey patch: https://gist.github.com/2130349. Any other issues?

José Valim
Owner

Thanks guys for your work. Could you please provide a pull request that reverts this merge and adds the fixes discussed above? I have also had bad experience with Identity Map in the past, but I will try it in my current project and see how it goes.

About @dolzenko's issue, I thought we already did that. Then they are probably other branches in the code adding an incomplete model to IM then.

Ryan Wallace

Okay I'll make a pull request. Thanks.

Sean Kirby sskirby referenced this pull request May 28, 2012
Closed

Identity map master #6524

Josep Jaume Rey josepjaume referenced this pull request in codegram/resort September 15, 2012
Closed

Identity Map #2

Sammy Larbi codeodor referenced this pull request in composite-primary-keys/composite_primary_keys May 11, 2013
Merged

Updated to work with Rails 4 release candidate 1 #154

zimbatm

Hi, is it possible to comment a bit on why the IdentityMap got removed from Rails ? I just got bit by that change today and was surprised that it wasn't explained more in the release notes and in the commit.

Collaborator

Because of some serious inconsistencies with associations. See #474

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.