Permalink
Browse files

Use separate Relation subclasses for each AR class

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.
  • Loading branch information...
1 parent c2be9b0 commit 64c53d7ce40006bdfea59102bdac4cb265d3ecd1 @jonleighton jonleighton committed Nov 23, 2012
@@ -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
@@ -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
@@ -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
@@ -6,46 +6,46 @@ 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
assert_equal({}, relation.create_with_value)
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,66 +110,66 @@ 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
assert_equal true, relation.readonly_value
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
assert_raises(ArgumentError) { Relation::HashMerger.new(nil, omg: 'lol') }
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
assert_not_nil relation.where_values
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|
@@ -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

3 comments on commit 64c53d7

Owner

tenderlove replied Aug 30, 2013

This commit breaks this test, but you cannot tell because the marshal dump / load take place in the same process. This commit breaks marshal transfer between two processes because the cached classes may not be generated by the time the other process tries to load.

Here is a self contained example:

require 'active_record'

class ActiveRecord::Base
  establish_connection 'sqlite3:///:memory:'
  connection.create_table :posts
  connection.create_table(:comments) { |t| t.references(:post) }
end

class Post < ActiveRecord::Base
  has_many :comments
end

class Comment < ActiveRecord::Base
  belongs_to :post
end

rd, wr = IO.pipe
fork {
  wr.close
  Marshal.load rd.read
}

post = Post.new
post.comments.build
wr.write Marshal.dump post

Here is the error:

[aaron@higgins rails (master)]$ ruby rawr.rb 
rawr.rb:22:in `load': undefined class/module ActiveRecord::Relation::ActiveRecord_Relation_Comment (ArgumentError)
    from rawr.rb:22:in `block in <main>'
    from rawr.rb:20:in `fork'
    from rawr.rb:20:in `<main>'
[aaron@higgins rails (master)]$
Owner

tenderlove replied Aug 31, 2013

Fixed here

Member

jonleighton replied Nov 10, 2013

@tenderlove bit late to this, but thanks for fixing!

Please sign in to comment.