Skip to content

Commit d9a54e4

Browse files
committed
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.
1 parent e386779 commit d9a54e4

File tree

7 files changed

+35
-17
lines changed

7 files changed

+35
-17
lines changed

activerecord/lib/active_record/associations.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2114,7 +2114,7 @@ def destroy_associations
21142114
end
21152115

21162116
has_many name, scope, **hm_options, &extension
2117-
_reflections[name.to_s].parent_reflection = habtm_reflection
2117+
_reflections[name].parent_reflection = habtm_reflection
21182118
end
21192119
end
21202120
end

activerecord/lib/active_record/associations/preloader/branch.rb

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,13 @@ class Branch # :nodoc:
99
attr_writer :preloaded_records
1010

1111
def initialize(association:, children:, parent:, associate_by_default:, scope:)
12-
@association = association
12+
@association = if association
13+
begin
14+
@association = association.to_sym
15+
rescue NoMethodError
16+
raise ArgumentError, "Association names must be Symbol or String, got: #{association.class.name}"
17+
end
18+
end
1319
@parent = parent
1420
@scope = scope
1521
@associate_by_default = associate_by_default

activerecord/lib/active_record/message_pack.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ def build_entry(record)
7979
end
8080

8181
def add_cached_associations(record, entry)
82-
record.class.reflections.each_value do |reflection|
82+
record.class.normalized_reflections.each_value do |reflection|
8383
if record.association_cached?(reflection.name)
8484
entry << reflection.name << encode(record.association(reflection.name).target)
8585
end

activerecord/lib/active_record/reflection.rb

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,12 @@ def create(macro, name, scope, options, ar)
2222

2323
def add_reflection(ar, name, reflection)
2424
ar.clear_reflections_cache
25-
name = -name.to_s
25+
name = name.to_sym
2626
ar._reflections = ar._reflections.except(name).merge!(name => reflection)
2727
end
2828

2929
def add_aggregate_reflection(ar, name, reflection)
30-
ar.aggregate_reflections = ar.aggregate_reflections.merge(-name.to_s => reflection)
30+
ar.aggregate_reflections = ar.aggregate_reflections.merge(name.to_sym => reflection)
3131
end
3232

3333
private
@@ -68,14 +68,18 @@ def reflect_on_all_aggregations
6868
# Account.reflect_on_aggregation(:balance) # => the balance AggregateReflection
6969
#
7070
def reflect_on_aggregation(aggregation)
71-
aggregate_reflections[aggregation.to_s]
71+
aggregate_reflections[aggregation.to_sym]
7272
end
7373

7474
# Returns a Hash of name of the reflection as the key and an AssociationReflection as the value.
7575
#
76-
# Account.reflections # => {"balance" => AggregateReflection}
76+
# Account.reflections # => {balance: => AggregateReflection}
7777
#
7878
def reflections
79+
normalized_reflections.stringify_keys
80+
end
81+
82+
def normalized_reflections # :nodoc
7983
@__reflections ||= begin
8084
ref = {}
8185

@@ -84,13 +88,13 @@ def reflections
8488

8589
if parent_reflection
8690
parent_name = parent_reflection.name
87-
ref[parent_name.to_s] = parent_reflection
91+
ref[parent_name] = parent_reflection
8892
else
8993
ref[name] = reflection
9094
end
9195
end
9296

93-
ref
97+
ref.freeze
9498
end
9599
end
96100

@@ -105,7 +109,7 @@ def reflections
105109
# Account.reflect_on_all_associations(:has_many) # returns an array of all has_many associations
106110
#
107111
def reflect_on_all_associations(macro = nil)
108-
association_reflections = reflections.values
112+
association_reflections = normalized_reflections.values
109113
association_reflections.select! { |reflection| reflection.macro == macro } if macro
110114
association_reflections
111115
end
@@ -116,16 +120,18 @@ def reflect_on_all_associations(macro = nil)
116120
# Invoice.reflect_on_association(:line_items).macro # returns :has_many
117121
#
118122
def reflect_on_association(association)
119-
reflections[association.to_s]
123+
normalized_reflections[association.to_sym]
120124
end
121125

122126
def _reflect_on_association(association) # :nodoc:
123-
_reflections[association.to_s]
127+
_reflections[association.to_sym]
124128
end
125129

126130
# Returns an array of AssociationReflection objects for all associations which have <tt>:autosave</tt> enabled.
127131
def reflect_on_all_autosave_associations
128-
reflections.values.select { |reflection| reflection.options[:autosave] }
132+
reflections = normalized_reflections.values
133+
reflections.select! { |reflection| reflection.options[:autosave] }
134+
reflections
129135
end
130136

131137
def clear_reflections_cache # :nodoc:
@@ -1159,7 +1165,7 @@ def check_validity!
11591165
end
11601166

11611167
if parent_reflection.nil?
1162-
reflections = active_record.reflections.keys.map(&:to_sym)
1168+
reflections = active_record.normalized_reflections.keys
11631169

11641170
if reflections.index(through_reflection.name) > reflections.index(name)
11651171
raise HasManyThroughOrderError.new(active_record.name, self, through_reflection)

activerecord/test/cases/associations/eager_test.rb

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1557,10 +1557,15 @@ def test_preloading_has_many_through_with_custom_scope
15571557
end
15581558

15591559
test "preload with invalid argument" do
1560-
exception = assert_raises(ActiveRecord::AssociationNotFoundError) do
1560+
exception = assert_raises(ArgumentError) do
15611561
Author.preload(10).to_a
15621562
end
1563-
assert_match(/Association named '10' was not found on Author; perhaps you misspelled it\?/, exception.message)
1563+
assert_match(/Association names must be Symbol or String, got: Integer/, exception.message)
1564+
1565+
exception = assert_raises(ActiveRecord::AssociationNotFoundError) do
1566+
Author.preload(:does_not_exists).to_a
1567+
end
1568+
assert_match(/Association named 'does_not_exists' was not found on Author; perhaps you misspelled it\?/, exception.message)
15641569
end
15651570

15661571
test "associations with extensions are not instance dependent" do

activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -928,7 +928,7 @@ def test_redefine_habtm
928928
end
929929

930930
def test_habtm_with_reflection_using_class_name_and_fixtures
931-
assert_not_nil Developer._reflections["shared_computers"]
931+
assert_not_nil Developer._reflections[:shared_computers]
932932
# Checking the fixture for named association is important here, because it's the only way
933933
# we've been able to reproduce this bug
934934
assert_not_nil File.read(File.expand_path("../../fixtures/developers.yml", __dir__)).index("shared_computers")

activerecord/test/cases/autosave_association_test.rb

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,7 @@ def test_cyclic_autosaves_do_not_add_multiple_validations
170170
private
171171
def assert_no_difference_when_adding_callbacks_twice_for(model, association_name)
172172
reflection = model.reflect_on_association(association_name)
173+
assert_not_nil reflection
173174
assert_no_difference "callbacks_for_model(#{model.name}).length" do
174175
model.send(:add_autosave_association_callbacks, reflection)
175176
end

0 commit comments

Comments
 (0)