From d9a54e41ab147a7e98559ffddda8ffc0fc059daa Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Fri, 3 May 2024 08:54:56 +0200 Subject: [PATCH] Use symbols as keys for `_reflections` Using a simple benchmark such as: ```ruby Post.transaction do 100.times do post = Post.create!(author_id: 42, title: "A" * 50, body: "a" * 400, tags: "blah blah blah", published_at: Time.now, comments_count: 20) 20.times do post.comments.create!(author_id: 42, body: "a" * 120, tags: "blah blah blah", published_at: Time.now) end end end posts = Post.includes(:comments).to_a ``` This allocate `43,077` objects, and out of these `2,200` are caused by association names being stored as strings internally: ``` Allocated String Report ----------------------------------- 80.00 kB 2000 "post" 2000 activerecord/lib/active_record/reflection.rb:123 8.00 kB 200 "comments" 200 activerecord/lib/active_record/reflection.rb:123 ``` This is because many API accept either symbol or string, and blindly call `to_s` on it. We could avoid this with sprinkling the code with `Symbol == association ? association.name : association.to_s`, but it's ugly. This issue may be entirely solved in a future Ruby version, but it will take years: https://bugs.ruby-lang.org/issues/20350 By using symbols, we both save allocations, and marginally speed up lookups and comparisons, reducing this particular benchmark allocations by 5%. It's a bit unclear why these were ever made strings. Historically symbols were immortal and using them for user supplied data could lead to DOS vulnerability, so this may have been why, but it's not longer a concern since Ruby 2.2. --- .../lib/active_record/associations.rb | 2 +- .../associations/preloader/branch.rb | 8 +++++- .../lib/active_record/message_pack.rb | 2 +- activerecord/lib/active_record/reflection.rb | 28 +++++++++++-------- .../test/cases/associations/eager_test.rb | 9 ++++-- ...s_and_belongs_to_many_associations_test.rb | 2 +- .../test/cases/autosave_association_test.rb | 1 + 7 files changed, 35 insertions(+), 17 deletions(-) diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index eff38203e55..d29ee604a5c 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -2114,7 +2114,7 @@ module ActiveRecord end has_many name, scope, **hm_options, &extension - _reflections[name.to_s].parent_reflection = habtm_reflection + _reflections[name].parent_reflection = habtm_reflection end end end diff --git a/activerecord/lib/active_record/associations/preloader/branch.rb b/activerecord/lib/active_record/associations/preloader/branch.rb index 4cd9bde50fc..13bac93dfde 100644 --- a/activerecord/lib/active_record/associations/preloader/branch.rb +++ b/activerecord/lib/active_record/associations/preloader/branch.rb @@ -9,7 +9,13 @@ module ActiveRecord attr_writer :preloaded_records def initialize(association:, children:, parent:, associate_by_default:, scope:) - @association = association + @association = if association + begin + @association = association.to_sym + rescue NoMethodError + raise ArgumentError, "Association names must be Symbol or String, got: #{association.class.name}" + end + end @parent = parent @scope = scope @associate_by_default = associate_by_default diff --git a/activerecord/lib/active_record/message_pack.rb b/activerecord/lib/active_record/message_pack.rb index b10f4ef6ea0..c9973dc3d8c 100644 --- a/activerecord/lib/active_record/message_pack.rb +++ b/activerecord/lib/active_record/message_pack.rb @@ -79,7 +79,7 @@ module ActiveRecord end def add_cached_associations(record, entry) - record.class.reflections.each_value do |reflection| + record.class.normalized_reflections.each_value do |reflection| if record.association_cached?(reflection.name) entry << reflection.name << encode(record.association(reflection.name).target) end diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index fb3dfadb495..486778574f5 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -22,12 +22,12 @@ module ActiveRecord def add_reflection(ar, name, reflection) ar.clear_reflections_cache - name = -name.to_s + name = name.to_sym ar._reflections = ar._reflections.except(name).merge!(name => reflection) end def add_aggregate_reflection(ar, name, reflection) - ar.aggregate_reflections = ar.aggregate_reflections.merge(-name.to_s => reflection) + ar.aggregate_reflections = ar.aggregate_reflections.merge(name.to_sym => reflection) end private @@ -68,14 +68,18 @@ module ActiveRecord # Account.reflect_on_aggregation(:balance) # => the balance AggregateReflection # def reflect_on_aggregation(aggregation) - aggregate_reflections[aggregation.to_s] + aggregate_reflections[aggregation.to_sym] end # Returns a Hash of name of the reflection as the key and an AssociationReflection as the value. # - # Account.reflections # => {"balance" => AggregateReflection} + # Account.reflections # => {balance: => AggregateReflection} # def reflections + normalized_reflections.stringify_keys + end + + def normalized_reflections # :nodoc @__reflections ||= begin ref = {} @@ -84,13 +88,13 @@ module ActiveRecord if parent_reflection parent_name = parent_reflection.name - ref[parent_name.to_s] = parent_reflection + ref[parent_name] = parent_reflection else ref[name] = reflection end end - ref + ref.freeze end end @@ -105,7 +109,7 @@ module ActiveRecord # Account.reflect_on_all_associations(:has_many) # returns an array of all has_many associations # def reflect_on_all_associations(macro = nil) - association_reflections = reflections.values + association_reflections = normalized_reflections.values association_reflections.select! { |reflection| reflection.macro == macro } if macro association_reflections end @@ -116,16 +120,18 @@ module ActiveRecord # Invoice.reflect_on_association(:line_items).macro # returns :has_many # def reflect_on_association(association) - reflections[association.to_s] + normalized_reflections[association.to_sym] end def _reflect_on_association(association) # :nodoc: - _reflections[association.to_s] + _reflections[association.to_sym] end # Returns an array of AssociationReflection objects for all associations which have :autosave enabled. def reflect_on_all_autosave_associations - reflections.values.select { |reflection| reflection.options[:autosave] } + reflections = normalized_reflections.values + reflections.select! { |reflection| reflection.options[:autosave] } + reflections end def clear_reflections_cache # :nodoc: @@ -1159,7 +1165,7 @@ module ActiveRecord end if parent_reflection.nil? - reflections = active_record.reflections.keys.map(&:to_sym) + reflections = active_record.normalized_reflections.keys if reflections.index(through_reflection.name) > reflections.index(name) raise HasManyThroughOrderError.new(active_record.name, self, through_reflection) diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb index 96b6a1df70c..77b0a995724 100644 --- a/activerecord/test/cases/associations/eager_test.rb +++ b/activerecord/test/cases/associations/eager_test.rb @@ -1557,10 +1557,15 @@ class EagerAssociationTest < ActiveRecord::TestCase end test "preload with invalid argument" do - exception = assert_raises(ActiveRecord::AssociationNotFoundError) do + exception = assert_raises(ArgumentError) do Author.preload(10).to_a end - assert_match(/Association named '10' was not found on Author; perhaps you misspelled it\?/, exception.message) + assert_match(/Association names must be Symbol or String, got: Integer/, exception.message) + + exception = assert_raises(ActiveRecord::AssociationNotFoundError) do + Author.preload(:does_not_exists).to_a + end + assert_match(/Association named 'does_not_exists' was not found on Author; perhaps you misspelled it\?/, exception.message) end test "associations with extensions are not instance dependent" do diff --git a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb index 40b4b0e40f4..8c8f8cd2efb 100644 --- a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb @@ -928,7 +928,7 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase end def test_habtm_with_reflection_using_class_name_and_fixtures - assert_not_nil Developer._reflections["shared_computers"] + assert_not_nil Developer._reflections[:shared_computers] # Checking the fixture for named association is important here, because it's the only way # we've been able to reproduce this bug assert_not_nil File.read(File.expand_path("../../fixtures/developers.yml", __dir__)).index("shared_computers") diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index 860bcc2e9c7..e872386967a 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -170,6 +170,7 @@ class TestAutosaveAssociationsInGeneral < ActiveRecord::TestCase private def assert_no_difference_when_adding_callbacks_twice_for(model, association_name) reflection = model.reflect_on_association(association_name) + assert_not_nil reflection assert_no_difference "callbacks_for_model(#{model.name}).length" do model.send(:add_autosave_association_callbacks, reflection) end