Skip to content

Commit

Permalink
Refactor Relation#none to no longer use Object#extend
Browse files Browse the repository at this point in the history
Ref: #47314
Ref: https://bugs.ruby-lang.org/issues/19436

`Object#extend` is really bad for performance as it generate an
infinite amount of unique classes from the point of view of the Ruby
virtual machine, so it keeps invalidating call caches.

Additionally, these call caches keep a reference on the last instance
they saw, which may lead to what is essentially a memory leak.
  • Loading branch information
byroot committed Mar 30, 2023
1 parent 80c51c1 commit 1d29f01
Show file tree
Hide file tree
Showing 8 changed files with 58 additions and 75 deletions.
1 change: 0 additions & 1 deletion activerecord/lib/active_record.rb
Expand Up @@ -99,7 +99,6 @@ module ActiveRecord
autoload :DisableJoinsAssociationRelation
autoload :FutureResult
autoload :LegacyYamlAdapter
autoload :NullRelation
autoload :Promise
autoload :Relation
autoload :Result
Expand Down
64 changes: 0 additions & 64 deletions activerecord/lib/active_record/null_relation.rb

This file was deleted.

27 changes: 23 additions & 4 deletions activerecord/lib/active_record/relation.rb
Expand Up @@ -35,6 +35,7 @@ def initialize(klass, table: klass.arel_table, predicate_builder: klass.predicat
@future_result = nil
@records = nil
@async = false
@none = false
end

def initialize_copy(other)
Expand Down Expand Up @@ -272,6 +273,8 @@ def size

# Returns true if there are no records.
def empty?
return true if @none

if loaded?
records.empty?
else
Expand All @@ -281,25 +284,33 @@ def empty?

# Returns true if there are no records.
def none?(*args)
return true if @none

return super if args.present? || block_given?
empty?
end

# Returns true if there are any records.
def any?(*args)
return false if @none

return super if args.present? || block_given?
!empty?
end

# Returns true if there is exactly one record.
def one?(*args)
return false if @none

return super if args.present? || block_given?
return records.one? if loaded?
limited_count == 1
end

# Returns true if there is more than one record.
def many?
return false if @none

return super if block_given?
return records.many? if loaded?
limited_count > 1
Expand Down Expand Up @@ -473,6 +484,8 @@ def _exec_scope(...) # :nodoc:
def update_all(updates)
raise ArgumentError, "Empty list of attributes to change" if updates.blank?

return 0 if @none

if updates.is_a?(Hash)
if klass.locking_enabled? &&
!updates.key?(klass.locking_column) &&
Expand Down Expand Up @@ -608,6 +621,8 @@ def destroy_all
# Post.distinct.delete_all
# # => ActiveRecord::ActiveRecordError: delete_all doesn't support distinct
def delete_all
return 0 if @none

invalid_methods = INVALID_METHODS_FOR_DELETE_ALL.select do |method|
value = @values[method]
method == :distinct ? value : value&.any?
Expand Down Expand Up @@ -851,10 +866,6 @@ def load_records(records)
@loaded = true
end

def null_relation? # :nodoc:
is_a?(NullRelation)
end

private
def already_in_scope?(registry)
@delegate_to_klass && registry.current_scope(klass, true)
Expand Down Expand Up @@ -939,6 +950,14 @@ def exec_queries(&block)
end

def exec_main_query(async: false)
if @none
if async
return Promise::Complete.new([])
else
return []
end
end

skip_query_cache_if_necessary do
if where_clause.contradiction?
[].freeze
Expand Down
17 changes: 16 additions & 1 deletion activerecord/lib/active_record/relation/calculations.rb
Expand Up @@ -188,10 +188,23 @@ def async_sum(identity_or_column = nil)
# ...
# end
def calculate(operation, column_name)
operation = operation.to_s.downcase

if @none
case operation
when "count", "sum"
result = group_values.any? ? Hash.new : 0
return @async ? Promise::Complete.new(result) : result
when "average", "minimum", "maximum"
result = group_values.any? ? Hash.new : nil
return @async ? Promise::Complete.new(result) : result
end
end

if has_include?(column_name)
relation = apply_join_dependency

if operation.to_s.downcase == "count"
if operation == "count"
unless distinct_value || distinct_select?(column_name || select_for_count)
relation.distinct!
relation.select_values = [ klass.primary_key || table[Arel.star] ]
Expand Down Expand Up @@ -241,6 +254,8 @@ def calculate(operation, column_name)
#
# See also #ids.
def pluck(*column_names)
return [] if @none

if loaded? && all_attributes?(column_names)
result = records.pluck(*column_names)
if @async
Expand Down
2 changes: 2 additions & 0 deletions activerecord/lib/active_record/relation/finder_methods.rb
Expand Up @@ -324,6 +324,8 @@ def second_to_last!
# Person.exists?
# Person.where(name: 'Spartacus', rating: 4).exists?
def exists?(conditions = :none)
return false if @none

if Base === conditions
raise ArgumentError, <<-MSG.squish
You are passing an instance of ActiveRecord::Base to `exists?`.
Expand Down
16 changes: 14 additions & 2 deletions activerecord/lib/active_record/relation/query_methods.rb
Expand Up @@ -1040,7 +1040,11 @@ def and!(other) # :nodoc:
#
def or(other)
if other.is_a?(Relation)
spawn.or!(other)
if @none
other.spawn
else
spawn.or!(other)
end
else
raise ArgumentError, "You have passed #{other.class.name} object to #or. Pass an ActiveRecord::Relation object instead."
end
Expand Down Expand Up @@ -1153,7 +1157,15 @@ def none
end

def none! # :nodoc:
where!("1=0").extending!(NullRelation)
unless @none
where!("1=0")
@none = true
end
self
end

def null_relation? # :nodoc:
@none
end

# Mark a relation as readonly. Attempting to update a record will result in
Expand Down
2 changes: 1 addition & 1 deletion activerecord/test/cases/null_relation_test.rb
Expand Up @@ -38,7 +38,7 @@ def test_async_query_on_null_relation
end

def test_none_chained_to_methods_firing_queries_straight_to_db
assert_no_queries do
assert_no_queries(ignore_none: false) do
assert_equal [], Developer.none.pluck(:id, :name)
assert_equal 0, Developer.none.delete_all
assert_equal 0, Developer.none.update_all(name: "David")
Expand Down
4 changes: 2 additions & 2 deletions activerecord/test/cases/relation/mutation_test.rb
Expand Up @@ -116,8 +116,8 @@ class RelationMutationTest < ActiveRecord::TestCase

test "none!" do
assert relation.none!.equal?(relation)
assert_equal [NullRelation], relation.extending_values
assert relation.is_a?(NullRelation)
assert_predicate relation, :none?
assert_predicate relation, :null_relation?
end

test "distinct!" do
Expand Down

0 comments on commit 1d29f01

Please sign in to comment.