mirror of https://github.com/rails/rails
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.
This commit is contained in:
parent
e3867798a0
commit
d9a54e41ab
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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 <tt>:autosave</tt> 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)
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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")
|
||||
|
|
|
@ -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
|
||||
|
|
Loading…
Reference in New Issue