From 64c53d7ce40006bdfea59102bdac4cb265d3ecd1 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Fri, 23 Nov 2012 12:36:22 +0000 Subject: [PATCH] Use separate Relation subclasses for each AR class MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At present, ActiveRecord::Delegation compiles delegation methods on a global basis. The compiled methods apply to all subsequent Relation instances. This creates several problems: 1) After Post.all.recent has been called, User.all.respond_to?(:recent) will be true, even if User.all.recent will actually raise an error due to no User.recent method existing. (See #8080.) 2) Depending on the AR class, the delegation should do different things. For example, if a Post.zip method exists, then Post.all.zip should call it. But this will then result in User.zip being called by a subsequent User.all.zip, even if User.zip does not exist, when in fact User.all.zip should call User.all.to_a.zip. (There are various variants of this problem.) We are creating these compiled delegations in order to avoid method missing and to avoid repeating logic on each invocation. One way of handling these issues is to add additional checks in various places to ensure we're doing the "right thing". However, this makes the compiled methods signficantly slower. In which case, there's almost no point in avoiding method_missing at all. (See #8127 for a proposed solution which takes this approach.) This is an alternative approach which involves creating a subclass of ActiveRecord::Relation for each AR class represented. So, with this patch, Post.all.class != User.all.class. This means that the delegations are compiled for and only apply to a single AR class. A compiled method for Post.all will not be invoked from User.all. This solves the above issues without incurring significant performance penalties. It's designed to be relatively seamless, however the downside is a bit of complexity and potentially confusion for a user who thinks that Post.all and User.all should be instances of the same class. Benchmark --------- require 'active_record' require 'benchmark/ips' class Post < ActiveRecord::Base establish_connection adapter: 'sqlite3', database: ':memory:' connection.create_table :posts def self.omg :omg end end relation = Post.all Benchmark.ips do |r| r.report('delegation') { relation.omg } r.report('constructing') { Post.all } end Before ------ Calculating ------------------------------------- delegation 4392 i/100ms constructing 4780 i/100ms ------------------------------------------------- delegation 144235.9 (±27.7%) i/s - 663192 in 5.038075s constructing 182015.5 (±21.2%) i/s - 850840 in 5.005364s After ----- Calculating ------------------------------------- delegation 6677 i/100ms constructing 6260 i/100ms ------------------------------------------------- delegation 166828.2 (±34.2%) i/s - 754501 in 5.001430s constructing 116575.5 (±18.6%) i/s - 563400 in 5.036690s Comments -------- Bear in mind that the standard deviations in the above are huge, so we can't compare the numbers too directly. However, we can conclude that Relation construction has become a little slower (as we'd expect), but not by a huge huge amount, and we can still construct a large number of Relations quite quickly. --- .../associations/collection_association.rb | 2 +- .../associations/collection_proxy.rb | 4 +- .../lib/active_record/relation/delegation.rb | 132 +++++++++++++----- activerecord/test/cases/relation_test.rb | 45 +++--- activerecord/test/cases/relations_test.rb | 29 ++++ 5 files changed, 156 insertions(+), 56 deletions(-) diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb index 1548e68cea..832b963052 100644 --- a/activerecord/lib/active_record/associations/collection_association.rb +++ b/activerecord/lib/active_record/associations/collection_association.rb @@ -34,7 +34,7 @@ def reader(force_reload = false) reload end - CollectionProxy.new(self) + CollectionProxy.new(klass, self) end # Implements the writer method, e.g. foo.items= for Foo.has_many :items diff --git a/activerecord/lib/active_record/associations/collection_proxy.rb b/activerecord/lib/active_record/associations/collection_proxy.rb index e444b0ed83..6e05af894e 100644 --- a/activerecord/lib/active_record/associations/collection_proxy.rb +++ b/activerecord/lib/active_record/associations/collection_proxy.rb @@ -30,9 +30,9 @@ module Associations class CollectionProxy < Relation delegate(*(ActiveRecord::Calculations.public_instance_methods - [:count]), to: :scope) - def initialize(association) #:nodoc: + def initialize(klass, association) #:nodoc: @association = association - super association.klass, association.klass.arel_table + super klass, klass.arel_table merge! association.scope(nullify: false) end diff --git a/activerecord/lib/active_record/relation/delegation.rb b/activerecord/lib/active_record/relation/delegation.rb index dbfa92bbbd..2184625e22 100644 --- a/activerecord/lib/active_record/relation/delegation.rb +++ b/activerecord/lib/active_record/relation/delegation.rb @@ -1,27 +1,113 @@ -require 'thread' +require 'active_support/concern' +require 'mutex_m' module ActiveRecord module Delegation # :nodoc: - # Set up common delegations for performance (avoids method_missing) + extend ActiveSupport::Concern + + # This module creates compiled delegation methods dynamically at runtime, which makes + # subsequent calls to that method faster by avoiding method_missing. The delegations + # may vary depending on the klass of a relation, so we create a subclass of Relation + # for each different klass, and the delegations are compiled into that subclass only. + delegate :to_xml, :to_yaml, :length, :collect, :map, :each, :all?, :include?, :to_ary, :to => :to_a delegate :table_name, :quoted_table_name, :primary_key, :quoted_primary_key, :connection, :columns_hash, :auto_explain_threshold_in_seconds, :to => :klass - @@delegation_mutex = Mutex.new + module ClassSpecificRelation + extend ActiveSupport::Concern - def self.delegate_to_scoped_klass(method) - if method.to_s =~ /\A[a-zA-Z_]\w*[!?]?\z/ - module_eval <<-RUBY, __FILE__, __LINE__ + 1 - def #{method}(*args, &block) - scoping { @klass.#{method}(*args, &block) } + included do + @delegation_mutex = Mutex.new + end + + module ClassMethods + def name + superclass.name + end + + def delegate_to_scoped_klass(method) + @delegation_mutex.synchronize do + return if method_defined?(method) + + if method.to_s =~ /\A[a-zA-Z_]\w*[!?]?\z/ + module_eval <<-RUBY, __FILE__, __LINE__ + 1 + def #{method}(*args, &block) + scoping { @klass.#{method}(*args, &block) } + end + RUBY + else + module_eval <<-RUBY, __FILE__, __LINE__ + 1 + def #{method}(*args, &block) + scoping { @klass.send(#{method.inspect}, *args, &block) } + end + RUBY + end end - RUBY - else - module_eval <<-RUBY, __FILE__, __LINE__ + 1 - def #{method}(*args, &block) - scoping { @klass.send(#{method.inspect}, *args, &block) } + end + + def delegate(method, opts = {}) + @delegation_mutex.synchronize do + return if method_defined?(method) + super end - RUBY + end + end + + protected + + def method_missing(method, *args, &block) + if @klass.respond_to?(method) + self.class.delegate_to_scoped_klass(method) + scoping { @klass.send(method, *args, &block) } + elsif Array.method_defined?(method) + self.class.delegate method, :to => :to_a + to_a.send(method, *args, &block) + elsif arel.respond_to?(method) + self.class.delegate method, :to => :arel + arel.send(method, *args, &block) + else + super + end + end + end + + module ClassMethods + # This hash is keyed by klass.name to avoid memory leaks in development mode + @@subclasses = Hash.new { |h, k| h[k] = {} }.extend(Mutex_m) + + def new(klass, *args) + relation = relation_class_for(klass).allocate + relation.__send__(:initialize, klass, *args) + relation + end + + # Cache the constants in @@subclasses because looking them up via const_get + # make instantiation significantly slower. + def relation_class_for(klass) + if klass && klass.name + if subclass = @@subclasses.synchronize { @@subclasses[self][klass.name] } + subclass + else + subclass = const_get("#{name.gsub('::', '_')}_#{klass.name.gsub('::', '_')}", false) + @@subclasses.synchronize { @@subclasses[self][klass.name] = subclass } + subclass + end + else + ActiveRecord::Relation + end + end + + # Check const_defined? in case another thread has already defined the constant. + # I am not sure whether this is strictly necessary. + def const_missing(name) + @@subclasses.synchronize { + if const_defined?(name) + const_get(name) + else + const_set(name, Class.new(self) { include ClassSpecificRelation }) + end + } end end @@ -35,28 +121,10 @@ def respond_to?(method, include_private = false) def method_missing(method, *args, &block) if @klass.respond_to?(method) - @@delegation_mutex.synchronize do - unless ::ActiveRecord::Delegation.method_defined?(method) - ::ActiveRecord::Delegation.delegate_to_scoped_klass(method) - end - end - scoping { @klass.send(method, *args, &block) } elsif Array.method_defined?(method) - @@delegation_mutex.synchronize do - unless ::ActiveRecord::Delegation.method_defined?(method) - ::ActiveRecord::Delegation.delegate method, :to => :to_a - end - end - to_a.send(method, *args, &block) elsif arel.respond_to?(method) - @@delegation_mutex.synchronize do - unless ::ActiveRecord::Delegation.method_defined?(method) - ::ActiveRecord::Delegation.delegate method, :to => :arel - end - end - arel.send(method, *args, &block) else super diff --git a/activerecord/test/cases/relation_test.rb b/activerecord/test/cases/relation_test.rb index 98e278df82..92dc575d37 100644 --- a/activerecord/test/cases/relation_test.rb +++ b/activerecord/test/cases/relation_test.rb @@ -6,26 +6,26 @@ module ActiveRecord class RelationTest < ActiveRecord::TestCase fixtures :posts, :comments - class FakeKlass < Struct.new(:table_name) + class FakeKlass < Struct.new(:table_name, :name) end def test_construction relation = nil assert_nothing_raised do - relation = Relation.new :a, :b + relation = Relation.new FakeKlass, :b end - assert_equal :a, relation.klass + assert_equal FakeKlass, relation.klass assert_equal :b, relation.table assert !relation.loaded, 'relation is not loaded' end def test_responds_to_model_and_returns_klass - relation = Relation.new :a, :b - assert_equal :a, relation.model + relation = Relation.new FakeKlass, :b + assert_equal FakeKlass, relation.model end def test_initialize_single_values - relation = Relation.new :a, :b + relation = Relation.new FakeKlass, :b (Relation::SINGLE_VALUE_METHODS - [:create_with]).each do |method| assert_nil relation.send("#{method}_value"), method.to_s end @@ -33,19 +33,19 @@ def test_initialize_single_values end def test_multi_value_initialize - relation = Relation.new :a, :b + relation = Relation.new FakeKlass, :b Relation::MULTI_VALUE_METHODS.each do |method| assert_equal [], relation.send("#{method}_values"), method.to_s end end def test_extensions - relation = Relation.new :a, :b + relation = Relation.new FakeKlass, :b assert_equal [], relation.extensions end def test_empty_where_values_hash - relation = Relation.new :a, :b + relation = Relation.new FakeKlass, :b assert_equal({}, relation.where_values_hash) relation.where! :hello @@ -79,7 +79,7 @@ def test_table_name_delegates_to_klass end def test_scope_for_create - relation = Relation.new :a, :b + relation = Relation.new FakeKlass, :b assert_equal({}, relation.scope_for_create) end @@ -110,31 +110,31 @@ def test_scope_for_create_is_cached end def test_empty_eager_loading? - relation = Relation.new :a, :b + relation = Relation.new FakeKlass, :b assert !relation.eager_loading? end def test_eager_load_values - relation = Relation.new :a, :b + relation = Relation.new FakeKlass, :b relation.eager_load! :b assert relation.eager_loading? end def test_references_values - relation = Relation.new :a, :b + relation = Relation.new FakeKlass, :b assert_equal [], relation.references_values relation = relation.references(:foo).references(:omg, :lol) assert_equal ['foo', 'omg', 'lol'], relation.references_values end def test_references_values_dont_duplicate - relation = Relation.new :a, :b + relation = Relation.new FakeKlass, :b relation = relation.references(:foo).references(:foo) assert_equal ['foo'], relation.references_values end test 'merging a hash into a relation' do - relation = Relation.new :a, :b + relation = Relation.new FakeKlass, :b relation = relation.merge where: :lol, readonly: true assert_equal [:lol], relation.where_values @@ -142,7 +142,7 @@ def test_references_values_dont_duplicate end test 'merging an empty hash into a relation' do - assert_equal [], Relation.new(:a, :b).merge({}).where_values + assert_equal [], Relation.new(FakeKlass, :b).merge({}).where_values end test 'merging a hash with unknown keys raises' do @@ -150,7 +150,7 @@ def test_references_values_dont_duplicate end test '#values returns a dup of the values' do - relation = Relation.new(:a, :b).where! :foo + relation = Relation.new(FakeKlass, :b).where! :foo values = relation.values values[:where] = nil @@ -158,18 +158,18 @@ def test_references_values_dont_duplicate end test 'relations can be created with a values hash' do - relation = Relation.new(:a, :b, where: [:foo]) + relation = Relation.new(FakeKlass, :b, where: [:foo]) assert_equal [:foo], relation.where_values end test 'merging a single where value' do - relation = Relation.new(:a, :b) + relation = Relation.new(FakeKlass, :b) relation.merge!(where: :foo) assert_equal [:foo], relation.where_values end test 'merging a hash interpolates conditions' do - klass = stub + klass = stub_everything klass.stubs(:sanitize_sql).with(['foo = ?', 'bar']).returns('foo = bar') relation = Relation.new(klass, :b) @@ -179,8 +179,11 @@ def test_references_values_dont_duplicate end class RelationMutationTest < ActiveSupport::TestCase + class FakeKlass < Struct.new(:table_name, :name) + end + def relation - @relation ||= Relation.new :a, :b + @relation ||= Relation.new FakeKlass, :b end (Relation::MULTI_VALUE_METHODS - [:references, :extending]).each do |method| diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index c34aeaf925..0cd838c0b0 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -1452,4 +1452,33 @@ def test_presence assert_equal expected, result end end + + test "delegations do not leak to other classes" do + Topic.all.by_lifo + assert Topic.all.class.method_defined?(:by_lifo) + assert !Post.all.respond_to?(:by_lifo) + end + + class OMGTopic < ActiveRecord::Base + self.table_name = 'topics' + + def self.__omg__ + "omgtopic" + end + end + + test "delegations do not clash across classes" do + begin + class ::Array + def __omg__ + "array" + end + end + + assert_equal "array", Topic.all.__omg__ + assert_equal "omgtopic", OMGTopic.all.__omg__ + ensure + Array.send(:remove_method, :__omg__) + end + end end -- GitLab