Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Refactor inversing logic of CollectionAssociation
Follow-up to rails#40379, rails#42524, and rails#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 ```
- Loading branch information
1 parent
cc9d0b9
commit ed79453