From 77f7b2df3aa3b7eb318cc830f239fd5ec2a7f28f Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Sun, 28 Apr 2019 15:06:30 +0900 Subject: [PATCH] Remove `sync_with_transaction_state` to simplify code base 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 --- .../attribute_methods/before_type_cast.rb | 3 - .../active_record/attribute_methods/dirty.rb | 10 --- .../active_record/attribute_methods/read.rb | 1 - .../active_record/attribute_methods/write.rb | 2 - .../abstract/database_statements.rb | 8 +-- .../abstract/transaction.rb | 22 +++++-- activerecord/lib/active_record/core.rb | 3 - activerecord/lib/active_record/persistence.rb | 4 -- .../lib/active_record/transactions.rb | 44 ++----------- activerecord/test/cases/transactions_test.rb | 64 ++++++++----------- 10 files changed, 52 insertions(+), 109 deletions(-) diff --git a/activerecord/lib/active_record/attribute_methods/before_type_cast.rb b/activerecord/lib/active_record/attribute_methods/before_type_cast.rb index 4a7b6c60e5..d3156e8075 100644 --- a/activerecord/lib/active_record/attribute_methods/before_type_cast.rb +++ b/activerecord/lib/active_record/attribute_methods/before_type_cast.rb @@ -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 diff --git a/activerecord/lib/active_record/attribute_methods/dirty.rb b/activerecord/lib/active_record/attribute_methods/dirty.rb index bd157b305a..bf5cc82f7a 100644 --- a/activerecord/lib/active_record/attribute_methods/dirty.rb +++ b/activerecord/lib/active_record/attribute_methods/dirty.rb @@ -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) diff --git a/activerecord/lib/active_record/attribute_methods/read.rb b/activerecord/lib/active_record/attribute_methods/read.rb index 0a95dd48db..4ad3e97d1c 100644 --- a/activerecord/lib/active_record/attribute_methods/read.rb +++ b/activerecord/lib/active_record/attribute_methods/read.rb @@ -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 diff --git a/activerecord/lib/active_record/attribute_methods/write.rb b/activerecord/lib/active_record/attribute_methods/write.rb index 0e8a6f9ca0..bfe2abcfa1 100644 --- a/activerecord/lib/active_record/attribute_methods/write.rb +++ b/activerecord/lib/active_record/attribute_methods/write.rb @@ -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 diff --git a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb index ddf6403a43..795d1fbc56 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb @@ -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). diff --git a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb index 6d908304d9..b7c97f8725 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb @@ -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! diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb index a26673728a..eede350c43 100644 --- a/activerecord/lib/active_record/core.rb +++ b/activerecord/lib/active_record/core.rb @@ -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 diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb index ad2247bb83..d4779ac4c6 100644 --- a/activerecord/lib/active_record/persistence.rb +++ b/activerecord/lib/active_record/persistence.rb @@ -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 diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb index bce34b1597..cf0eeb6a88 100644 --- a/activerecord/lib/active_record/transactions.rb +++ b/activerecord/lib/active_record/transactions.rb @@ -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 @transaction_state 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 diff --git a/activerecord/test/cases/transactions_test.rb b/activerecord/test/cases/transactions_test.rb index 253a3ce0fa..9bdb4cb00d 100644 --- a/activerecord/test/cases/transactions_test.rb +++ b/activerecord/test/cases/transactions_test.rb @@ -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"