From ed79453a49d205dec8dd439c760f4fb191594a4e Mon Sep 17 00:00:00 2001 From: Jonathan Hefner Date: Sat, 18 Jun 2022 17:11:30 -0500 Subject: [PATCH] Refactor inversing logic of CollectionAssociation Follow-up to #40379, #42524, and #43445. The logic to associate records with a `CollectionAssociation` when using `has_many` inversing has become increasingly complex. Prior to this commit, we defined an "add record" method (`add_to_target`) that can accept a `replace: true` option, and a "replace record" method (`replace_on_target`) that can accept a `replace: false` option. And regardless of the value of the `:replace` option, the record may be added if it cannot be replaced (i.e. does not exist), or replaced if a complex set of conditions dictate it should not be simply added. This commit aims to simplify this logic by: * Removing the `:replace` option from `add_to_target`. * Renaming `replace_on_target` to a low-level `add_record` method, supporting a `:replace` option. The name `add_record` was chosen for parity with the similarly low-level `remove_records` method. * Replacing the complex conditions of `replace_on_target` with an "implicit target records" concept. Implicit target records are those that have been associated via inversing but not explicitly added. Implicit target records exist in the `@target` array, and are also tracked via an `@implicit_target_records` array. Whenever an implicit target record is explicitly added, it is always replaced in the `@target` array to prevent a double occurrence, and then removed from the `@implicit_target_records` array. (Thus if the record is explicitly added a 2nd time, a 2nd occurrence *will* be added to the `@target` array. We have tests that expect this.) Additionally, this commit provides a very small speed increase, according to the following benchmark: ```ruby require "benchmark" ActiveRecord::Base.logger = nil ActiveRecord::Schema.define do create_table :authors, force: true do |t| end create_table :posts, force: true do |t| t.references :author end end class Author < ActiveRecord::Base has_many :posts end class Post < ActiveRecord::Base belongs_to :author end POST_COUNTS = [1, 10, 100, 1000] ITERATIONS = 100 CALLS_PER_ITERATION = 100 eval <<~RUBY def ms_per_build(posts) Benchmark.ms { #{"posts.build;" * CALLS_PER_ITERATION} }.to_f / CALLS_PER_ITERATION end def ms_per_shovel(posts, post) Benchmark.ms { #{"posts << post;" * CALLS_PER_ITERATION} }.to_f / CALLS_PER_ITERATION end RUBY def report(label, &block) [false, true].each do |has_many_inversing| ActiveRecord::Base.has_many_inversing = has_many_inversing POST_COUNTS.each do |post_count| Post.delete_all Post.insert_all(post_count.times.map { { author_id: 1 } }) block.call # warmup ms = ITERATIONS.times.sum(&block).to_f / ITERATIONS puts "has_many_inversing == #{has_many_inversing}, posts.count == #{post_count}".ljust(50) + "=> `#{label}` @ #{"%.3f" % ms}ms" end end end author = Author.create!(id: 1) report("posts.build") do posts = author.reload.posts.tap(&:load_target) ms_per_build(posts) end report("posts << post") do posts = author.reload.posts.tap(&:load_target) post = posts[posts.length / 2] ms_per_shovel(posts, post) end ``` Results with `main` branch: ``` has_many_inversing == false, posts.count == 1 => `posts.build` @ 0.154ms has_many_inversing == false, posts.count == 10 => `posts.build` @ 0.142ms has_many_inversing == false, posts.count == 100 => `posts.build` @ 0.141ms has_many_inversing == false, posts.count == 1000 => `posts.build` @ 0.144ms has_many_inversing == true, posts.count == 1 => `posts.build` @ 0.143ms has_many_inversing == true, posts.count == 10 => `posts.build` @ 0.142ms has_many_inversing == true, posts.count == 100 => `posts.build` @ 0.142ms has_many_inversing == true, posts.count == 1000 => `posts.build` @ 0.147ms has_many_inversing == false, posts.count == 1 => `posts << post` @ 0.192ms has_many_inversing == false, posts.count == 10 => `posts << post` @ 0.192ms has_many_inversing == false, posts.count == 100 => `posts << post` @ 0.202ms has_many_inversing == false, posts.count == 1000 => `posts << post` @ 0.196ms has_many_inversing == true, posts.count == 1 => `posts << post` @ 0.209ms has_many_inversing == true, posts.count == 10 => `posts << post` @ 0.204ms has_many_inversing == true, posts.count == 100 => `posts << post` @ 0.221ms has_many_inversing == true, posts.count == 1000 => `posts << post` @ 0.195ms ``` Results with this branch: ``` has_many_inversing == false, posts.count == 1 => `posts.build` @ 0.144ms has_many_inversing == false, posts.count == 10 => `posts.build` @ 0.132ms has_many_inversing == false, posts.count == 100 => `posts.build` @ 0.133ms has_many_inversing == false, posts.count == 1000 => `posts.build` @ 0.135ms has_many_inversing == true, posts.count == 1 => `posts.build` @ 0.132ms has_many_inversing == true, posts.count == 10 => `posts.build` @ 0.133ms has_many_inversing == true, posts.count == 100 => `posts.build` @ 0.132ms has_many_inversing == true, posts.count == 1000 => `posts.build` @ 0.140ms has_many_inversing == false, posts.count == 1 => `posts << post` @ 0.200ms has_many_inversing == false, posts.count == 10 => `posts << post` @ 0.187ms has_many_inversing == false, posts.count == 100 => `posts << post` @ 0.188ms has_many_inversing == false, posts.count == 1000 => `posts << post` @ 0.191ms has_many_inversing == true, posts.count == 1 => `posts << post` @ 0.187ms has_many_inversing == true, posts.count == 10 => `posts << post` @ 0.188ms has_many_inversing == true, posts.count == 100 => `posts << post` @ 0.197ms has_many_inversing == true, posts.count == 1000 => `posts << post` @ 0.189ms ``` --- .../associations/collection_association.rb | 45 ++++++++----------- 1 file changed, 18 insertions(+), 27 deletions(-) diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb index ea31b122b5cbe..3b852ab234aac 100644 --- a/activerecord/lib/active_record/associations/collection_association.rb +++ b/activerecord/lib/active_record/associations/collection_association.rb @@ -85,7 +85,7 @@ def ids_writer(ids) def reset super @target = [] - @replaced_or_added_targets = Set.new.compare_by_identity + @implicit_target_records = nil @association_ids = nil end @@ -116,7 +116,7 @@ def build(attributes = nil, &block) if attributes.is_a?(Array) attributes.collect { |attr| build(attr, &block) } else - add_to_target(build_record(attributes, &block), replace: true) + add_to_target(build_record(attributes, &block)) end end @@ -274,21 +274,19 @@ def load_target target end - def add_to_target(record, skip_callbacks: false, replace: false, &block) - replace_on_target(record, skip_callbacks, replace: replace || association_scope.distinct_value, &block) + def add_to_target(record, skip_callbacks: false, &block) + add_record(record, replace: association_scope.distinct_value, skip_callbacks: skip_callbacks, &block) end - def target=(record) - return super unless reflection.klass.has_many_inversing + def inversed_from(record) + # When removing a record from an inverse association, `inversed_from` is + # called with nil. But we need the record itself in order to remove it, + # so instead we ignore. + add_record(record, implicit: true, skip_callbacks: true) if record + end - case record - when nil - # It's not possible to remove the record from the inverse association. - when Array - super - else - replace_on_target(record, true, replace: true, inversing: true) - end + def inversed_from_queries(record) + inversed_from(record) if inversable?(record) end def scope @@ -395,6 +393,7 @@ def remove_records(existing_records, records, method) delete_records(existing_records, method) if existing_records.any? @target -= records + @implicit_target_records -= records if @implicit_target_records @association_ids = nil records.each { |record| callback(:after_remove, record) } @@ -422,8 +421,7 @@ def replace_records(new_target, original_target) def replace_common_records_in_memory(new_target, original_target) common_records = intersection(new_target, original_target) common_records.each do |record| - skip_callbacks = true - replace_on_target(record, skip_callbacks, replace: true) + add_record(record, replace: true, skip_callbacks: true) end end @@ -446,11 +444,7 @@ def concat_records(records, raise = false) records end - def replace_on_target(record, skip_callbacks, replace:, inversing: false) - if replace && (!record.new_record? || @replaced_or_added_targets.include?(record)) - index = @target.index(record) - end - + def add_record(record, replace: false, implicit: false, skip_callbacks: false) catch(:abort) do callback(:before_add, record) end || return unless skip_callbacks @@ -461,13 +455,10 @@ def replace_on_target(record, skip_callbacks, replace:, inversing: false) yield(record) if block_given? - if !index && @replaced_or_added_targets.include?(record) - index = @target.index(record) - end - - @replaced_or_added_targets << record if inversing || index || record.new_record? + was_implicit = @implicit_target_records&.delete(record) + (@implicit_target_records ||= []) << record if implicit - if index + if (replace || was_implicit) && index = target.index(record) target[index] = record elsif @_was_loaded || !loaded? @association_ids = nil