Permalink
Browse files

More thread safety. Refactor association registration, add logic to o…

…nly add associations for prefetching that are used by more than 25% of fetched records
  • Loading branch information...
1 parent e17b794 commit aab8082dd553997c38a2fb668740cb2f0d875643 @sdsykes sdsykes committed Mar 30, 2009
View
@@ -5,7 +5,7 @@ class Callsite
# associations referenced at the callsite.
#
- Mtx = Mutex.new
+ Mtx = Mutex.new # mutex should perhaps be per-instance at the expense of a little memory
attr_accessor :klass,
:signature,
@@ -20,19 +20,14 @@ def initialize( klass, signature )
# Flag a column as seen
#
def column!( column )
- Mtx.synchronize do
- columns << column
- end
+ columns && Mtx.synchronize { @columns << column }
end
# Flag an association as seen
#
def association!(association, record_id)
if preloadable_association?(association)
- Mtx.synchronize do
- associations << association
- end
- register_record_id(record_id)
+ associations.register(association, record_id)
end
end
@@ -43,40 +38,48 @@ def inspect
# Lazy init default columns
#
def default_columns
- @default_columns ||= setup_columns
+ @default_columns || Mtx.synchronize { @default_columns = setup_columns }
end
# Lazy init columns
#
def columns
- @columns ||= default_columns.dup
- end
+ @columns || default_columns && Mtx.synchronize { @columns = @default_columns.dup }
+ end
# Lazy init associations
#
def associations
- @associations ||= setup_associations
+ @associations || Mtx.synchronize { @associations = setup_associations }
+ end
+
+ def has_associations?
+ @associations
end
# Analyze previously collected information
# and reset ready for a new query
#
def reset
- # placeholder
+ if has_associations?
+ associations.reset
+ end
end
def register_result_set(result_set)
- # placeholder
- end
-
- def register_record_id(record_id)
- # placeholder
+ if has_associations?
+ associations.register_result_set(result_set)
+ end
end
private
def associations_for_inspect
- associations.map{|a| ":#{a.to_s}" }.join(', ')
+ if has_associations?
+ associations.to_preload.map{|a| ":#{a.to_s}" }.join(', ')
+ else
+ ""
+ end
end
# Only register associations that isn't polymorphic or a collection
@@ -105,7 +108,7 @@ def setup_columns
# Start with no registered associations
#
def setup_associations
- Set.new
+ Optimizations::Associations::AssociationSet.new
end
# Memoize a string representation of the inheritance column
@@ -0,0 +1,96 @@
+module Scrooge
+ module Optimizations
+ module Associations
+
+ # Keeps track of how a result set is used to access associations
+ # Each callsite will contain one of these objects.
+ # Each thread will collect data, and we check this data before each
+ # fetch from the database, adding any associations that are needed
+ # which are returned when to_preload is called.
+ #
+ # Note the the association set is only made by scrooge when an
+ # association is accessed, so the first time through the code
+ # data is not collected because we did not record the result set size.
+ #
+ class AssociationSet
+
+ Mtx = Mutex.new
+
+ def initialize
+ @associations = Set.new
+ @as_data_id = :"association_data_#{object_id}"
+ end
+
+ def register(association, record_id)
+ assoc_data.register(association, record_id)
+ end
+
+ def register_result_set(result_set)
+ assoc_data.register_result_set(result_set)
+ end
+
+ def reset
+ Mtx.synchronize do
+ @associations |= assoc_data.to_preload
+ end
+ assoc_data.reset
+ end
+
+ def to_preload
+ @associations.to_a
+ end
+
+ private
+
+ def assoc_data
+ Thread.current[@as_data_id] ||= AssociationData.new
+ end
+ end
+
+ class AssociationData
+ def initialize
+ reset
+ end
+
+ def reset
+ @associations = Set.new
+ @accessed_via = {}
+ @result_set_size = 0
+ end
+
+ def register(association, record_id)
+ if @result_set_size > 1
+ @associations << association
+ @accessed_via[association] ||= []
+ @accessed_via[association] << record_id
+ end
+ end
+
+ def register_result_set(result_set)
+ @result_set_size = result_set.size
+ end
+
+ def to_preload
+ @associations.select { |association| preload_this_assoc?(association) }
+ end
+
+ private
+
+ # Calculate the benefit of preloading an association
+ # There is no benefit if result set is just one record
+ # Otherwise we look at how many of the result set items were used
+ # to access the association - more than 25% and we preload
+ #
+ # TODO: more rules and analysis for different association types
+ #
+ def preload_this_assoc?(association)
+ if @result_set_size <= 1
+ false
+ else
+ @accessed_via[association].size > @result_set_size / 4
+ end
+ end
+ end
+ end
+ end
+end
@@ -11,7 +11,7 @@ def install!
unless scrooge_installed?
ActiveRecord::Base.send( :extend, SingletonMethods )
ActiveRecord::Associations::AssociationProxy.send( :include, InstanceMethods )
- end
+ end
end
protected
@@ -36,9 +36,11 @@ def self.extended( base )
def preload_scrooge_associations(result_set, callsite_sig)
if result_set.size > 1
scrooge_preloading_exclude do
- callsite_associations = scrooge_callsite(callsite_sig).associations
- unless callsite_associations.empty?
- preload_associations(result_set, callsite_associations)
+ if scrooge_callsite(callsite_sig).has_associations?
+ callsite_associations = scrooge_callsite(callsite_sig).associations.to_preload
+ unless callsite_associations.empty?
+ preload_associations(result_set, callsite_associations)
+ end
end
end
end
@@ -87,9 +87,7 @@ def find_by_sql_with_scrooge(sql)
callsite.register_result_set(result_set)
- if Associations::Macro.scrooge_installed?
- preload_scrooge_associations(result_set, callsite_sig)
- end
+ preload_scrooge_associations(result_set, callsite_sig)
result_set
end
View
@@ -5,6 +5,7 @@
require 'optimizations/columns/attributes_proxy'
require 'optimizations/columns/macro'
require 'optimizations/associations/macro'
+require 'optimizations/associations/association_set'
require 'optimizations/result_sets/updateable_result_set'
require 'optimizations/result_sets/result_array'
@@ -13,9 +14,14 @@ class Base
@@scrooge_callsites = {}
ScroogeCallsiteSample = 0..10
+ ScroogeMutex = Mutex.new
+
+ # this can be set to help with testing
+ cattr_accessor :scrooge_ignore_call_stack
+ @@scrooge_ignore_call_stack = ($0 == "irb")
class << self
-
+
# Determine if a given SQL string is a candidate for callsite <=> columns
# optimization.
#
@@ -30,28 +36,33 @@ def find_by_sql(sql)
# Expose known callsites for this model
#
def scrooge_callsites
- @@scrooge_callsites[self.table_name] ||= {}
+ @@scrooge_callsites[table_name] || ScroogeMutex.synchronize { @@scrooge_callsites[table_name] = {} }
end
# Fetch or setup a callsite instance for a given signature
#
def scrooge_callsite( callsite_signature )
- @@scrooge_callsites[self.table_name] ||= {}
- @@scrooge_callsites[self.table_name][callsite_signature] ||= callsite( callsite_signature )
+ scrooge_callsites[callsite_signature] || ScroogeMutex.synchronize do
+ scrooge_callsites[callsite_signature] = callsite(callsite_signature)
+ end
end
- # Flush all known callsites.Mostly a test helper.
- #
+ # Flush all known callsites. Mostly a test helper.
+ #
def scrooge_flush_callsites!
- @@scrooge_callsites[self.table_name] = {}
+ ScroogeMutex.synchronize do
+ @@scrooge_callsites[table_name] = {}
+ end
end
private
# Removes a single callsite
#
def scrooge_unlink_callsite!( callsite_signature )
- @@scrooge_callsites.delete(callsite_signature)
+ ScroogeMutex.synchronize do
+ @@scrooge_callsites.delete(callsite_signature)
+ end
end
# Initialize a callsite
@@ -70,7 +81,11 @@ def attribute_with_table( attr_name )
# context information.
#
def callsite_signature( call_stack, supplementary )
- ( call_stack[ScroogeCallsiteSample] << supplementary ).hash
+ if @@scrooge_ignore_call_stack
+ supplementary.hash
+ else
+ ( call_stack[ScroogeCallsiteSample] << supplementary ).hash
+ end
end
end # class << self
View
@@ -17,7 +17,7 @@ def setup
end
test "should be inspectable" do
- @callsite.association! :mysql_user
+ @callsite.association! :mysql_user, 123456
@callsite.column! :db
assert_equal @callsite.inspect, "<#MysqlTablePrivilege :select => '`tables_priv`.db', :include => [:mysql_user]>"
end
@@ -29,12 +29,12 @@ def setup
end
test "should flag only preloadable associations as seen" do
- assert_no_difference '@callsite.associations.size' do
- @callsite.association! :undefined
+ assert_no_difference '@callsite.associations.to_preload.size' do
+ @callsite.association! :undefined, 123456
end
- assert_difference '@callsite.associations.size', 2 do
- @callsite.association! :column_privilege
- @callsite.association! :mysql_user
+ assert_difference '@callsite.associations.to_preload.size', 2 do
+ @callsite.association! :column_privilege, 123456
+ @callsite.association! :mysql_user, 123456
end
end
@@ -7,14 +7,14 @@ class OptimizationsAssociationsMacroTest < ActiveSupport::TestCase
test "should be able to flag any associations instantiated from a record" do
@user = MysqlUser.find(:first)
@user.host
- assert_equal MysqlUser.scrooge_callsite( @user.callsite_signature ).associations, Set[:host]
+ assert_equal MysqlUser.scrooge_callsite( @user.callsite_signature ).associations.to_preload, [:host]
end
test "should only flag preloadable associations" do
Scrooge::Callsite.any_instance.expects(:association!).once
@user = MysqlUser.find(:first)
@user.host
- assert_equal MysqlUser.scrooge_callsite( @user.callsite_signature ).associations, Set.new
+ assert_equal [], MysqlUser.scrooge_callsite( @user.callsite_signature ).associations.to_preload
end
test "should be able to identify all preloadable associations for a given Model" do

0 comments on commit aab8082

Please sign in to comment.