1
0
Fork 0
mirror of https://github.com/rails/rails.git synced 2022-11-09 12:12:34 -05:00

Revert "create a transaction object and point AR objects at that object during a"

This reverts commit c24c885209.

Here's the explanation I just sent to @tenderlove:

Hey,

I've been thinking about about the transaction memory leak thing that we
were discussing.

Example code:

post = nil
Post.transaction do
  N.times { post = Post.create }
end

Post.transaction is going to create a real transaction and there will
also be a (savepoint) transaction inside each Post.create.

In an idea world, we'd like all but the last Post instance to be GC'd,
and for the last Post instance to receive its after_commit callback when
Post.transaction returns.

I can't see how this can work using your solution where the Post itself
holds a reference to the transaction it is in; when Post.transaction
returns, control does not switch to any of Post's instance methods, so
it can't trigger the callbacks itself.

What we really want is for the transaction itself to hold weak
references to the objects within the transaction. So those objects can
be GC'd, but if they are not GC'd then the transaction can iterate them
and execute their callbacks.

I've looked into WeakRef implementations that are available. On 1.9.3,
the stdlib weakref library is broken and we shouldn't use it.

There is a better implementation here:

https://github.com/bdurand/ref/blob/master/lib/ref/weak_reference/pure_ruby.rb

We could use that, either by pulling in the gem or just copying the code
in, but it still suffers from the limitation that it uses ObjectSpace
finalizers.

In my testing, this finalizers make GC quite expensive:
https://gist.github.com/3722432

Ruby 2.0 will have a native WeakRef implementation (via
ObjectSpace::WeakMap), hence won't be reliant on finalizers:
http://bugs.ruby-lang.org/issues/4168

So the ultimate solution will be for everyone to use Ruby 2.0, and for
us to just use ObjectSpace::WeakMap.

In the meantime, we have basically 3 options:

The first is to leave it as it is.

The second is to use a finalizer-based weakref implementation and take
the GC perf hit.

The final option is to store object ids rather than the actual objects.
Then use ObjectSpace._id2ref to deference the objects at the end of the
transaction, if they exist. This won't stop memory use growing within
the transaction, but it'll grow more slowly.

I benchmarked the performance of _id2ref this if the object does or does
not exist: https://gist.github.com/3722550

If it does exist it seems decent, but it's hugely more expensive if it
doesn't, probably because we have to do the rescue nil.

Probably most of the time the objects will exist. However the point of
doing this optimisation is to allow people to create a large number of
objects inside a transaction and have them be GC'd. So for that use
case, we'd be replacing one problem with another. I'm not sure which of
the two problems is worse.

My feeling is that we should just leave this for now and come back to it
when Ruby 2.0 is out.

I'm going to revert your commit because I can't see how it solves this.
Hope you don't mind... if I've misunderstood then let me know!

Jon
This commit is contained in:
Jon Leighton 2012-09-14 16:44:35 +01:00
parent 8577687fcb
commit b89ffe7f00
4 changed files with 12 additions and 55 deletions

View file

@ -193,8 +193,7 @@ module ActiveRecord
rescue Exception => database_transaction_rollback rescue Exception => database_transaction_rollback
if transaction_open && !outside_transaction? if transaction_open && !outside_transaction?
transaction_open = false transaction_open = false
txn = decrement_open_transactions decrement_open_transactions
txn.aborted!
if open_transactions == 0 if open_transactions == 0
rollback_db_transaction rollback_db_transaction
rollback_transaction_records(true) rollback_transaction_records(true)
@ -209,10 +208,9 @@ module ActiveRecord
@transaction_joinable = last_transaction_joinable @transaction_joinable = last_transaction_joinable
if outside_transaction? if outside_transaction?
@current_transaction = nil @open_transactions = 0
elsif transaction_open elsif transaction_open
txn = decrement_open_transactions decrement_open_transactions
txn.committed!
begin begin
if open_transactions == 0 if open_transactions == 0
commit_db_transaction commit_db_transaction

View file

@ -69,7 +69,6 @@ module ActiveRecord
@last_use = false @last_use = false
@logger = logger @logger = logger
@open_transactions = 0 @open_transactions = 0
@current_transaction = nil
@pool = pool @pool = pool
@query_cache = Hash.new { |h,sql| h[sql] = {} } @query_cache = Hash.new { |h,sql| h[sql] = {} }
@query_cache_enabled = false @query_cache_enabled = false
@ -237,30 +236,14 @@ module ActiveRecord
@connection @connection
end end
def open_transactions attr_reader :open_transactions
count = 0
txn = current_transaction
while txn
count += 1
txn = txn.next
end
count
end
attr_reader :current_transaction
def increment_open_transactions def increment_open_transactions
@current_transaction = Transaction.new(current_transaction) @open_transactions += 1
end end
def decrement_open_transactions def decrement_open_transactions
return unless current_transaction @open_transactions -= 1
txn = current_transaction
@current_transaction = txn.next
txn
end end
def transaction_joinable=(joinable) def transaction_joinable=(joinable)

View file

@ -387,7 +387,6 @@ module ActiveRecord
@marked_for_destruction = false @marked_for_destruction = false
@new_record = true @new_record = true
@mass_assignment_options = nil @mass_assignment_options = nil
@txn = nil
@_start_transaction_state = {} @_start_transaction_state = {}
end end
end end

View file

@ -1,32 +1,6 @@
require 'thread' require 'thread'
module ActiveRecord module ActiveRecord
class Transaction
attr_reader :next
def initialize(txn = nil)
@next = txn
@committed = false
@aborted = false
end
def committed!
@committed = true
end
def aborted!
@aborted = true
end
def committed?
@committed
end
def aborted?
@aborted
end
end
# See ActiveRecord::Transactions::ClassMethods for documentation. # See ActiveRecord::Transactions::ClassMethods for documentation.
module Transactions module Transactions
extend ActiveSupport::Concern extend ActiveSupport::Concern
@ -333,11 +307,11 @@ module ActiveRecord
def with_transaction_returning_status def with_transaction_returning_status
status = nil status = nil
self.class.transaction do self.class.transaction do
@txn = self.class.connection.current_transaction
add_to_transaction add_to_transaction
begin begin
status = yield status = yield
rescue ActiveRecord::Rollback rescue ActiveRecord::Rollback
@_start_transaction_state[:level] = (@_start_transaction_state[:level] || 0) - 1
status = nil status = nil
end end
@ -353,17 +327,20 @@ module ActiveRecord
@_start_transaction_state[:id] = id if has_attribute?(self.class.primary_key) @_start_transaction_state[:id] = id if has_attribute?(self.class.primary_key)
@_start_transaction_state[:new_record] = @new_record @_start_transaction_state[:new_record] = @new_record
@_start_transaction_state[:destroyed] = @destroyed @_start_transaction_state[:destroyed] = @destroyed
@_start_transaction_state[:level] = (@_start_transaction_state[:level] || 0) + 1
end end
# Clear the new record state and id of a record. # Clear the new record state and id of a record.
def clear_transaction_record_state #:nodoc: def clear_transaction_record_state #:nodoc:
@_start_transaction_state.clear if @txn.committed? @_start_transaction_state[:level] = (@_start_transaction_state[:level] || 0) - 1
@_start_transaction_state.clear if @_start_transaction_state[:level] < 1
end end
# Restore the new record state and id of a record that was previously saved by a call to save_record_state. # Restore the new record state and id of a record that was previously saved by a call to save_record_state.
def restore_transaction_record_state(force = false) #:nodoc: def restore_transaction_record_state(force = false) #:nodoc:
unless @_start_transaction_state.empty? unless @_start_transaction_state.empty?
if @txn.aborted? || force @_start_transaction_state[:level] = (@_start_transaction_state[:level] || 0) - 1
if @_start_transaction_state[:level] < 1 || force
restore_state = @_start_transaction_state restore_state = @_start_transaction_state
was_frozen = @attributes.frozen? was_frozen = @attributes.frozen?
@attributes = @attributes.dup if was_frozen @attributes = @attributes.dup if was_frozen