Reduce memory bloat in ActiveRecord transactions #776

Closed
lighthouse-import opened this Issue May 16, 2011 · 20 comments

6 participants

@lighthouse-import

Imported from Lighthouse. Original ticket at: http://rails.lighthouseapp.com/projects/8994/tickets/6129
Created by Brian Durand - 2011-04-14 10:17:34 UTC

This patch reduces memory bloat in ActiveRecord transactions. With the introduction after_commit and after_rollback callbacks references are kept to all records updated in a transaction until the transaction completes. This can lead to memory bloat with large transactions.

The code change replaces the references to weak references unless the objects implement these callbacks. This allows the garbage collector to reclaim objects that won't be executing a callback.

@lighthouse-import

Imported from Lighthouse.
Comment by Piotr Sarnacki - 2010-12-11 10:47:18 UTC

I can't remember correctly what was wrong with WeakRef, but during development of IdentitiyMap, something new was implemented (ActiveSupport::WeakHash) and Weakling was used for JRuby: miloops@ada0149

Also you might want to talk to Emilio Tagua, he probably has some more details on that.

@lighthouse-import

Imported from Lighthouse.
Comment by Piotr Sarnacki - 2010-12-11 10:53:02 UTC

Just to be clear about IM stuff - it hasn't been merged into master yet, here is the pull request for that: #76

@lighthouse-import

Imported from Lighthouse.
Comment by Jacek Becela - 2010-12-11 11:30:12 UTC

I like the WeakRef solution very much - works for me(tm) but I don't use anything other than MRI. +1.

@lighthouse-import

Imported from Lighthouse.
Comment by Brian Durand - 2010-12-13 16:48:13 UTC

It looks to me like the identity map stuff is implementing a weak HashMap and in the jruby case utilizing the existing Java weak reference libraries. I don't see anything in the ActiveSupport::WeakHashMap or the Weakling code that would indicate there was a problem with how WeakRef itself is implemented. They both seem to be doing the same thing internally the WeakRef is doing.

This patch doesn't need a weak hash map or a reference queue so I think it should be fine with WeakRef. The weak reference exist only to support the callbacks that reset the state of new or deleted objects if the transaction fails. If the objects have been garbage collected because there are no hard references to them, then there is no need to invoke these callbacks.

@lighthouse-import

Imported from Lighthouse.
Comment by Aaron Patterson - 2010-12-13 18:43:27 UTC

The problem is that WeakRef uses _id2ref to look up an object. _id2ref takes an object id, and returns the object corresponding with that id. The problem is that object ids are not unique in MRI, the get reused. That means that you could get back an object that isn't the object you were actually looking for.

We can easily demonstrate id reuse like so:

class Foo; end
class Bar; end

id_to_class = {}

loop do
  obj = [Foo, Bar].sort_by { rand }.first.new

  if id_to_class.key? obj.object_id
    puts "obj id: #{obj.object_id} was reused"

    if id_to_class[obj.object_id] != obj.class
      puts "omg! they aren't even the same class!"
    end
  end

  id_to_class[obj.object_id] = obj.class
end

Now imagine that two AR objects had object_ids that collided.

I hesitate to apply this patch because of this problem. Maybe we can find a different way?

@lighthouse-import

Imported from Lighthouse.
Comment by Brian Durand - 2010-12-16 03:16:22 UTC

I've thoroughly looked into the WeakRef implementation and how it works across the various Ruby runtimes and this is what I've found: it sucks.

  1. Object ids are not reused in Jruby, Rubinius, or IronRuby so WeakRefs do work properly (makes sense since these all run on a VM).
  2. Object ids are reused in MRI 1.8.7 and REE 1.8.7 do reuse object ids, but the WeakRef implementation handles that case properly because of the single system thread the process is running on.
  3. Object ids are reused in MRI 1.9.2 but the WeakRef implementation does not handle this properly since the garbage collector can run concurrently with threads allocating new objects. This messes up cleanup logic since finalizers attached to an object id can be run after that object id has been assigned to a new object.
  4. The WeakRef implementation does not scale on most runtimes. Performance on MRI 1.8.7, REE 1.8.7, and IronRuby is especially bad (performance on Rubinius is actually good, though). For some reason it extends Delegator which redefines all of the wrapped object's methods on initialization. This is both expensive on the CPU and can take up a lot of memory. On MRI 1.8.7 creating an array of 50,000 WeakRefs that wrap Object.new will balloon the heap to well over 1GB and creating each WeakRef takes 15-100 times longer to do. I have no idea why you would want to use a Delegator on a WeakRef unless you want your code to randomly break when the garbage collector kicks in, but it just doesn't seem worth it.
  5. The weakling gem provides a better WeakRef implementation for Jruby that uses the native Java weak references.

Since WeakRef is either broken or performs horribly on 5 of the 6 Ruby runtimes I tested, and it is a really useful feature to have, I think it makes sense to add it to ActiveSupport so we can have a consistent working interface for weak references. Unfortunately, easier said than done. I started out simply reimplementing WeakRef to make it not extend Delegator and fix the bugs with 1.9. However, Jruby has it's own better implementation and since Rubinius has a very efficient implementation of WeakRef, but doesn't perform at all well with finalizers. Thus, I ended up with three different implementations of WeakReference and the runtime picks the best choice:

  1. Jruby uses one backed by Weakling if it is available otherwise it uses one backed by WeakRef
  2. Rubinius uses the implementation backed by WeakRef
  3. All other runtimes use my reimplementation of WeakRef

The interface is very simple:

obj = Object.new
ref = ActiveSupport::WeakReference.new(obj)
ref.object                # obj or nil if the reference has been reclaimed
ref.referenced_object_id  # obj.object_id

Further, I used this implementation to create a WeakHash class where the values are weak references. This could be used to replace the WeakHash shown in the commit linked to above for the identity map stuff.

Attached is a new patch with the original ActiveRecord changes along with the WeakReference implementations. Also attached is my test script in case anyone wants to verify the bizarre weak reference hell I've stumbled into. To run the test script, simply run it with your favorite flavor of ruby and pass one of object, weakref, or weakreference as the argument:

ruby weakref_test.rb object         # baseline object creation without weak references
ruby weakref_test.rb weakref        # uses WeakRef
ruby weakref_test.rb weakreference  # uses new ActiveSupport::WeakReference

Each test will report on how long it takes to create 1000 instances of the specified class and then will run 100,000 iterations trying to find reused object id's. Be careful if you run the weakref tests under MRI, REE, or IronRuby because your process heap will grow to 2+GB and your computer may become severely unresponsive.

@lighthouse-import

Imported from Lighthouse.
Comment by Aaron Patterson - 2010-12-16 03:43:52 UTC

@Brian Excellent work! The implementation looks great. Is there a reason this should be in ActiveSupport? IMO, a weakref / weakhash library would be useful outside rails.

Also, if you create a gem outside rails, you can create JRuby specific versions that depend on weakling, which guarantees that weakling is available.

Finally, shouldn't we be filing bug reports / patches against MRI?

What do you think?

@lighthouse-import

Imported from Lighthouse.
Comment by Brian Durand - 2010-12-16 15:47:26 UTC

@Aaron Totally agree that WeakRef needs to be fixed in Ruby 1.9 branch and I will submit a patch for MRI. As for usefulness outside of Rails I also totally concur.

My original thought on adding it to ActiveSupport is that it fits in as some generally useful functionality that is either missing from core Ruby or not universally available like Base64, Gzip, OrderedHash, TimeWithZone, etc. I do, however, agree that the best method for sharing code like this in this day in age that has nothing to do with Rails (other than I need it to fix the ActiveRecord transaction issue) is as a separate gem. I can work on turning the patch code gem into a gem.

Before I do that, though, what are the thoughts on adding another gem dependency to Rails for this purpose? Is that the preferred direction for adding new generic functionality vs. updating ActiveSupport with new features?

@lighthouse-import

Imported from Lighthouse.
Comment by Aaron Patterson - 2010-12-16 19:34:11 UTC

@Brian Excellent. Ping me when you submit the patch to MRI and I will help convince the other ruby-core members.

I prefer that this library would be in a gem, and I think I can convince the rails-core people that it is necessary. Releasing the weakref code as a gem would be beneficial because we can release the gem out of band with rails. Also, we can change dependencies depending on the target platform (like make this gem depend on weakling if the gem is installing on jruby).

Have you thought about folding this code in to weakling?

If you need any help, let me know. I think weakrefs are the correct way to fix this issue, so I'm happy to help.

@lighthouse-import

Imported from Lighthouse.
Comment by Brian Durand - 2010-12-17 16:59:27 UTC

@Aaron I've submitted a patch for Ruby 1.9

http://redmine.ruby-lang.org/issues/show/4168

@lighthouse-import

Imported from Lighthouse.
Comment by Brian Durand - 2011-01-11 16:41:20 UTC

I have released a new "ref" gem (http://rubygems.org/gems/ref) that provides working weak references for all the major ruby implementations. I have tested it on MRI, REE, YARV, Rubinius, Jruby, and IronRuby.

I improved and expanded on my previous code to provide support for soft references, reference queues, and four different kinds of reference maps. Since someone previously mentioned that work going on to implement an identity map looked at weak references, they might find this code useful.

Attached is an updated ActiveRecord patch that uses the new gem.

@lighthouse-import

Imported from Lighthouse.
Comment by rails - 2011-04-12 00:00:08 UTC

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.

@lighthouse-import

Imported from Lighthouse.
Comment by Brian Durand - 2011-04-13 16:32:33 UTC

Still an issue. This can be a serious issue where many records are updated or deleted within a transaction since the ActiveRecord objects tend to have large memory footprints and can quickly bloat the memory beyond what is available.

[state:open]

@spastorino spastorino reopened this Jun 4, 2011
@jake3030 jake3030 pushed a commit to jake3030/rails that referenced this issue Jun 28, 2011
@dhh dhh Update the default deprecation message to not promise that theres mor…
…e info at the Rails site [#776 state:resolved]
ea2545f
@willbryant

Hang on a minute - when an activerecord class doesn't implement after_commit or after_rollback, we should ask why do we need to make even a weak reference to it?

I believe the answer is that when we do a rollback from 3.0 on there is code that rolls back the state of all objects saved within the transaction (not just those that were in the save call stack at the time the exception that causes the rollback occurs, as 2.3 did).

To do that it needs to hold a reference, and holding only a weak reference would just mean that this functionality sometimes breaks.

That's a feature, and while I personally don't need that feature at all - and definitely need the framework to not hold on to all objects in the transaction, which is a huge bloating problem for my app - I'm sure there's people who rely on this feature by now. 2.3 not doing that was considered a longstanding bug by a couple of people.

So problems with the weak reference implementation don't seem the real issue to me. The issue is that there is a direct conflict between the current feature set and what we need to do in order not to bloat.

Here's a two-line patch you can use to try this out:

diff --git a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.r
index 2750ca0..b3f6713 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
@@ -188,7 +188,8 @@ module ActiveRecord
       # can be called.
       def add_transaction_record(record)
         last_batch = @_current_transaction_records.last
-        last_batch << record if last_batch
+        last_batch << record if last_batch && !(record._commit_callbacks.empty? && record._rollback_callbacks.empty?)
+        last_batch
       end

Which produces the following test failures:

  1) Failure:
test_callback_rollback_in_create(TransactionTest)
    [/Users/will/rails/activerecord/test/cases/transactions_test.rb:198:in `test_callback_rollback_in_create'
     /Users/will/rails/activerecord/test/cases/transactions_test.rb:190:in `times'
     /Users/will/rails/activerecord/test/cases/transactions_test.rb:190:in `test_callback_rollback_in_create'
     /Users/will/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:67:in `__send__'
     /Users/will/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:67:in `run'
     /Users/will/rails/activesupport/lib/active_support/callbacks.rb:419:in `_run_setup_callbacks'
     /Users/will/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:65:in `run']:
The topic should have its old new_record value.
<true> expected but was
<false>.

  2) Failure:
test_restore_active_record_state_for_all_records_in_a_transaction(TransactionTest)
    [/Users/will/rails/activerecord/test/cases/transactions_test.rb:362:in `test_restore_active_record_state_for_all_records_in_a_transaction'
     /Users/will/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:67:in `__send__'
     /Users/will/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:67:in `run'
     /Users/will/rails/activesupport/lib/active_support/callbacks.rb:419:in `_run_setup_callbacks'
     /Users/will/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:65:in `run']:
<true> expected but was
<false>.

It's easy to make the first test pass using code like we had in 2.3 - rollback your state if you were in a #save call and it raises - without relying on the current add_to_transaction list, but the second test there is testing out this behavior of rolling back the state of any other objects that completed their own save earlier in the transaction.

Unless I've missed something fundamental (in which case apologies), the only reason the weak ref patch didn't show up these failures was because GC didn't happen to run in between the weak reference being made and the rollback occurring. But the point of weak refs is that it can, and a nondeterministic solution is off the cards.

So I think we would need to have a way of declaring somewhere that for this transaction or this record save we don't care about rolling back object states if the transaction is rolled back later. This is a very ugly solution, and not doing this by default is bound to catch people out regularly, but at least we can use it when required to stop our apps bloating.

@willbryant

Scratch that - I think I see now. We don't care about rolling back the AR state if no-one holds a strong reference, because no-one should care about the side effects of rolling back state on an object they no longer reference?

@tenderlove tenderlove was assigned Jul 9, 2011
@isaacsanders

Is this still an issue?

@willbryant

Believe so - it was the last time I tested.

@willbryant

BTW - see my pull request #3300 (#3300), which has a patch I use to work around this on a large rails site. Works nicely, and doesn't change the default behavior.

@rafaelfranca
Ruby on Rails member

Closing it since #9068 was merged.

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