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

Remove sync_with_transaction_state to simplify code base

Browse source 
This also removes the `if @transaction_state&.finalized?` guard which is
harder to understand optimization introduced at #36049. The guard is
faster enough though, but removing that will make attribute access about
2% ~ 4% faster, and will make code base to ease to maintain.

`sync_with_transaction_state` was introduced at #9068 to address memory
bloat when creating lots of AR objects inside a transaction.

I've found #18638 the same design of this to address memory bloat, but
this differs from #18638 in that it will allocate one `WeakMap` object
only when explicit transaction, no extra allocation for implicit
transaction.

Executable script to reproduce memory bloat:

https://gist.github.com/kamipo/36d869fff81cf878658adc26ee38ea1b
https://github.com/rails/rails/issues/15549#issuecomment-46035848

I can see no memory concern with this.

Co-authored-by: Arthur Neves <arthurnn@gmail.com>
This commit is contained in:
Ryuta Kamizono 2019-04-28 15:06:30 +09:00
parent c0c53ee9d2
commit 77f7b2df3a
10 changed files with 52 additions and 109 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
activerecord/lib/active_record/attribute_methods/before_type_cast.rb
View file

@ -46,7 +46,6 @@ module ActiveRecord
# task.read_attribute_before_type_cast('completed_on') # => "2012-10-21"
# task.read_attribute_before_type_cast(:completed_on) # => "2012-10-21"
def read_attribute_before_type_cast(attr_name)
sync_with_transaction_state if @transaction_state&.finalized?
@attributes[attr_name.to_s].value_before_type_cast
end
@ -61,7 +60,6 @@ module ActiveRecord
# task.attributes_before_type_cast
# # => {"id"=>nil, "title"=>nil, "is_done"=>true, "completed_on"=>"2012-10-21", "created_at"=>nil, "updated_at"=>nil}
def attributes_before_type_cast
sync_with_transaction_state if @transaction_state&.finalized?
@attributes.values_before_type_cast
end
@ -72,7 +70,6 @@ module ActiveRecord
end
def attribute_came_from_user?(attribute_name)
sync_with_transaction_state if @transaction_state&.finalized?
@attributes[attribute_name].came_from_user?
end
end

10
activerecord/lib/active_record/attribute_methods/dirty.rb
View file

@ -156,16 +156,6 @@ module ActiveRecord
end
private
def mutations_from_database
sync_with_transaction_state if @transaction_state&.finalized?
super
end
def mutations_before_last_save
sync_with_transaction_state if @transaction_state&.finalized?
super
end
def write_attribute_without_type_cast(attr_name, value)
result = super
clear_attribute_change(attr_name)

1
activerecord/lib/active_record/attribute_methods/read.rb
View file

@ -33,7 +33,6 @@ module ActiveRecord
# This method exists to avoid the expensive primary_key check internally, without
# breaking compatibility with the read_attribute API
def _read_attribute(attr_name, &block) # :nodoc
sync_with_transaction_state if @transaction_state&.finalized?
@attributes.fetch_value(attr_name.to_s, &block)
end

2
activerecord/lib/active_record/attribute_methods/write.rb
View file

@ -37,14 +37,12 @@ module ActiveRecord
# This method exists to avoid the expensive primary_key check internally, without
# breaking compatibility with the write_attribute API
def _write_attribute(attr_name, value) # :nodoc:
sync_with_transaction_state if @transaction_state&.finalized?
@attributes.write_from_user(attr_name.to_s, value)
value
end
private
def write_attribute_without_type_cast(attr_name, value)
sync_with_transaction_state if @transaction_state&.finalized?
@attributes.write_cast_value(attr_name.to_s, value)
value
end

8
activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
View file

@ -333,12 +333,8 @@ module ActiveRecord
# Register a record with the current transaction so that its after_commit and after_rollback callbacks
# can be called.
def add_transaction_record(record)
current_transaction.add_record(record)
end
def transaction_state
current_transaction.state
def add_transaction_record(record, ensure_finalize = true)
current_transaction.add_record(record, ensure_finalize)
end
# Begins the transaction (and turns off auto-committing).

22
activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
View file

@ -70,11 +70,11 @@ module ActiveRecord
def closed?; true; end
def open?; false; end
def joinable?; false; end
def add_record(record); end
def add_record(record, _ = true); end
end
class Transaction #:nodoc:
attr_reader :connection, :state, :records, :savepoint_name, :isolation_level
attr_reader :connection, :state, :savepoint_name, :isolation_level
def initialize(connection, isolation: nil, joinable: true, run_commit_callbacks: false)
@connection = connection
@ -84,11 +84,25 @@ module ActiveRecord
@materialized = false
@joinable = joinable
@run_commit_callbacks = run_commit_callbacks
@lazy_enrollment_records = nil
end
def add_record(record)
def add_record(record, ensure_finalize = true)
@records ||= []
@records << record
if ensure_finalize
@records << record
else
@lazy_enrollment_records ||= ObjectSpace::WeakMap.new
@lazy_enrollment_records[record] = record
end
end
def records
if @lazy_enrollment_records
@records.concat @lazy_enrollment_records.values
@lazy_enrollment_records = nil
end
@records
end
def materialize!

3
activerecord/lib/active_record/core.rb
View file

@ -415,7 +415,6 @@ module ActiveRecord
@previously_new_record = false
@destroyed = false
@_start_transaction_state = nil
@transaction_state = nil
super
end
@ -475,7 +474,6 @@ module ActiveRecord
# Returns +true+ if the attributes hash has been frozen.
def frozen?
sync_with_transaction_state if @transaction_state&.finalized?
@attributes.frozen?
end
@ -594,7 +592,6 @@ module ActiveRecord
@marked_for_destruction = false
@destroyed_by_association = nil
@_start_transaction_state = nil
@transaction_state = nil
@strict_loading = false
self.class.define_attribute_methods

4
activerecord/lib/active_record/persistence.rb
View file

@ -424,7 +424,6 @@ module ActiveRecord
# Returns true if this object hasn't been saved yet -- that is, a record
# for the object doesn't exist in the database yet; otherwise, returns false.
def new_record?
sync_with_transaction_state if @transaction_state&.finalized?
@new_record
end
@ -432,20 +431,17 @@ module ActiveRecord
# save, the object didn't exist in the database and new_record? would have
# returned true.
def previously_new_record?
sync_with_transaction_state if @transaction_state&.finalized?
@previously_new_record
end
# Returns true if this object has been destroyed, otherwise returns false.
def destroyed?
sync_with_transaction_state if @transaction_state&.finalized?
@destroyed
end
# Returns true if the record is persisted, i.e. it's not a new record and it was
# not destroyed, otherwise returns false.
def persisted?
sync_with_transaction_state if @transaction_state&.finalized?
!(@new_record || @destroyed)
end

44
activerecord/lib/active_record/transactions.rb
View file

@ -342,13 +342,11 @@ module ActiveRecord
# instance.
def with_transaction_returning_status
status = nil
self.class.transaction do
if has_transactional_callbacks?
add_to_transaction
else
sync_with_transaction_state if @transaction_state&.finalized?
@transaction_state = self.class.connection.transaction_state
end
connection = self.class.connection
ensure_finalize = !connection.transaction_open?
connection.transaction do
add_to_transaction(ensure_finalize || has_transactional_callbacks?)
remember_transaction_record_state
status = yield
@ -395,7 +393,6 @@ module ActiveRecord
# Force to clear the transaction record state.
def force_clear_transaction_record_state
@_start_transaction_state = nil
@transaction_state = nil
end
# Restore the new record state and id of a record that was previously saved by a call to save_record_state.
@ -436,39 +433,12 @@ module ActiveRecord
# Add the record to the current transaction so that the #after_rollback and #after_commit
# callbacks can be called.
def add_to_transaction
self.class.connection.add_transaction_record(self)
def add_to_transaction(ensure_finalize = true)
self.class.connection.add_transaction_record(self, ensure_finalize)
end
def has_transactional_callbacks?
!_rollback_callbacks.empty? || !_commit_callbacks.empty? || !_before_commit_callbacks.empty?
end
# Updates the attributes on this particular Active Record object so that
# if it's associated with a transaction, then the state of the Active Record
# object will be updated to reflect the current state of the transaction.
#
# The <tt>@transaction_state</tt> variable stores the states of the associated
# transaction. This relies on the fact that a transaction can only be in
# one rollback or commit (otherwise a list of states would be required).
# Each Active Record object inside of a transaction carries that transaction's
# TransactionState.
#
# This method checks to see if the ActiveRecord object's state reflects
# the TransactionState, and rolls back or commits the Active Record object
# as appropriate.
def sync_with_transaction_state
if transaction_state = @transaction_state
if transaction_state.fully_committed?
force_clear_transaction_record_state
elsif transaction_state.committed?
clear_transaction_record_state
elsif transaction_state.rolledback?
force_restore_state = transaction_state.fully_rolledback?
restore_transaction_record_state(force_restore_state)
clear_transaction_record_state
end
end
end
end
end

64
activerecord/test/cases/transactions_test.rb
View file

@ -99,13 +99,11 @@ class TransactionTest < ActiveRecord::TestCase
def test_raise_after_destroy
assert_not_predicate @first, :frozen?
assert_not_called(@first, :rolledback!) do
assert_raises(RuntimeError) do
Topic.transaction do
@first.destroy
assert_predicate @first, :frozen?
raise
end
assert_raises(RuntimeError) do
Topic.transaction do
@first.destroy
assert_predicate @first, :frozen?
raise
end
end
@ -113,13 +111,11 @@ class TransactionTest < ActiveRecord::TestCase
end
def test_successful
assert_not_called(@first, :committed!) do
Topic.transaction do
@first.approved = true
@second.approved = false
@first.save
@second.save
end
Topic.transaction do
@first.approved = true
@second.approved = false
@first.save
@second.save
end
assert_predicate Topic.find(1), :approved?, "First should have been approved"
@ -152,10 +148,8 @@ class TransactionTest < ActiveRecord::TestCase
end
end
assert_not_called(@first, :committed!) do
assert_deprecated do
transaction_with_return
end
assert_deprecated do
transaction_with_return
end
assert committed
@ -194,11 +188,9 @@ class TransactionTest < ActiveRecord::TestCase
end
end
assert_not_called(@first, :committed!) do
Topic.transaction do
@first.approved = true
@first.save!
end
Topic.transaction do
@first.approved = true
@first.save!
end
assert_equal 0, num
@ -210,13 +202,11 @@ class TransactionTest < ActiveRecord::TestCase
end
def test_successful_with_instance_method
assert_not_called(@first, :committed!) do
@first.transaction do
@first.approved = true
@second.approved = false
@first.save
@second.save
end
@first.transaction do
@first.approved = true
@second.approved = false
@first.save
@second.save
end
assert_predicate Topic.find(1), :approved?, "First should have been approved"
@ -224,7 +214,7 @@ class TransactionTest < ActiveRecord::TestCase
end
def test_failing_on_exception
assert_not_called(@first, :rolledback!) do
begin
Topic.transaction do
@first.approved = true
@second.approved = false
@ -249,10 +239,8 @@ class TransactionTest < ActiveRecord::TestCase
end
@first.approved = true
assert_not_called(@first, :rolledback!) do
e = assert_raises(RuntimeError) { @first.save }
assert_equal "Make the transaction rollback", e.message
end
e = assert_raises(RuntimeError) { @first.save }
assert_equal "Make the transaction rollback", e.message
assert_not_predicate Topic.find(1), :approved?
end
@ -278,10 +266,8 @@ class TransactionTest < ActiveRecord::TestCase
raise "Make the transaction rollback"
end
assert_not_called(topic, :rolledback!) do
assert_raises(RuntimeError) do
Topic.transaction { topic.save }
end
assert_raises(RuntimeError) do
Topic.transaction { topic.save }
end
assert_predicate topic, :new_record?, "#{topic.inspect} should be new record"