Change the way the tags and params are handled in rails

The rationale behind this patch is that it takes a lots of time
to let rails unserialize the ParamValue and ResourceTag object
on each compilation, just to throw them away the second after.
The idea is to fetch directly (and batched host per host) the
parameters and tags from the database and then returns them as
hash.
This allows the no-modification case to takes at least 2 times
less than before.

Signed-off-by: Brice Figureau <brice-puppet@daysofwonder.com>

4 files changed, 105 insertions, 41 deletions

diff --git a/lib/puppet/parser/resource.rb b/lib/puppet/parser/resource.rb
index 747338b3b..2fdd78ddf 100644
--- a/lib/puppet/parser/resource.rb
+++ b/lib/puppet/parser/resource.rb
@@ -181,11 +181,11 @@ class Puppet::Parser::Resource
             hash
         end
         
-        db_resource.ar_hash_merge(db_resource.get_params_hash(db_resource.param_values), updated_params,
+        db_resource.ar_hash_merge(db_resource.get_params_hash(), updated_params,
                                   :create => Proc.new { |name, parameter|
                                       parameter.to_rails(db_resource)
                                   }, :delete => Proc.new { |values|
-                                      values.each { |value| db_resource.param_values.delete(value) }
+                                      values.each { |value| Puppet::Rails::ParamValue.delete(value['id']) }
                                   }, :modify => Proc.new { |db, mem|
                                       mem.modify_rails_values(db)
                                   })
@@ -194,13 +194,13 @@ class Puppet::Parser::Resource
             hash[tag] = tag
             hash
         }
-            
+
         db_resource.ar_hash_merge(db_resource.get_tag_hash(), 
                                   updated_tags,
                                   :create => Proc.new { |name, tag|
                                       db_resource.add_resource_tag(name)
                                   }, :delete => Proc.new { |tag|
-                                      db_resource.resource_tags.delete(tag)
+                                      Puppet::Rails::ResourceTag.delete(tag['id'])
                                   }, :modify => Proc.new { |db, mem|
                                       # nothing here
                                   })
@@ -437,7 +437,7 @@ class Puppet::Parser::Resource
             # 'type' isn't a valid column name, so we have to use another name.
             to = (param == :type) ? :restype : param
             if value = self.send(param)
-                hash[to] = value 
+                hash[to] = value
             end
             hash
         end
diff --git a/lib/puppet/parser/resource/param.rb b/lib/puppet/parser/resource/param.rb
index c8dd78a26..1a5cfe8fb 100644
--- a/lib/puppet/parser/resource/param.rb
+++ b/lib/puppet/parser/resource/param.rb
@@ -12,7 +12,7 @@ class Puppet::Parser::Resource::Param
     end
 
     def inspect
-        "#<#{self.class} @name => #{name}, @value => #{value}, @source => #{source.name}>"
+        "#<#{self.class} @name => #{name}, @value => #{value}, @source => #{source.name}, @line => #{line}>"
     end
 
     def line_to_i
@@ -39,7 +39,6 @@ class Puppet::Parser::Resource::Param
 
         param_name = Puppet::Rails::ParamName.find_or_create_by_name(self.name.to_s)
         line_number = line_to_i()
-
         return values.collect do |v|
             db_resource.param_values.create(:value => v,
                                            :line => line_number,
@@ -48,18 +47,18 @@ class Puppet::Parser::Resource::Param
     end
 
     def modify_rails_values(db_values)
-        #dev_warn if db_values.nil? || db_values.empty? 
+        #dev_warn if db_values.nil? || db_values.empty?
 
         values_to_remove(db_values).each { |remove_me|
-            Puppet::Rails::ParamValue.delete(remove_me.id)
+            Puppet::Rails::ParamValue.delete(remove_me['id'])
         }
         line_number = line_to_i()
-        values_to_add(db_values).each { |add_me| 
-            db_resource = db_values[0].resource
-            db_param_name = db_values[0].param_name
-            db_resource.param_values.create(:value => add_me,
+        db_param_name = db_values[0]['param_name_id']
+        values_to_add(db_values).each { |add_me|
+            Puppet::Rails::ParamValue.create(:value => add_me,
                                            :line => line_number,
-                                           :param_name => db_param_name)
+                                           :param_name_id => db_param_name,
+                                           :resource_id => db_values[0]['resource_id'] )
         }
     end
     
@@ -79,9 +78,9 @@ class Puppet::Parser::Resource::Param
         values = munge_for_rails(value)
         line_number = line_to_i()
         db_values.collect do |db|
-            db unless (db.line == line_number && 
+            db unless (db['line'] == line_number && 
                        values.find { |v| 
-                         compare(v,db.value)
+                         compare(v,db['value'])
                        } )
         end.compact
     end
@@ -90,8 +89,8 @@ class Puppet::Parser::Resource::Param
         values = munge_for_rails(value)
         line_number = line_to_i()
         values.collect do |v|
-            v unless db_values.find { |db| (compare(v,db.value) && 
-                                         line_number == db.line) }
+            v unless db_values.find { |db| (compare(v,db['value']) && 
+                                         line_number == db['line']) }
         end.compact
     end
 end
diff --git a/lib/puppet/rails/host.rb b/lib/puppet/rails/host.rb
index 187dc657a..a429cbfb1 100644
--- a/lib/puppet/rails/host.rb
+++ b/lib/puppet/rails/host.rb
@@ -3,7 +3,7 @@ require 'puppet/rails/fact_name'
 require 'puppet/rails/source_file'
 require 'puppet/util/rails/collection_merger'
 
-# Puppet::TIME_DEBUG = true
+Puppet::TIME_DEBUG = true
 
 class Puppet::Rails::Host < ActiveRecord::Base
     include Puppet::Util
@@ -12,9 +12,7 @@ class Puppet::Rails::Host < ActiveRecord::Base
     has_many :fact_values, :dependent => :destroy
     has_many :fact_names, :through => :fact_values
     belongs_to :source_file
-    has_many :resources,
-        :include => :param_values,
-        :dependent => :destroy
+    has_many :resources, :dependent => :destroy
 
     # If the host already exists, get rid of its objects
     def self.clean(host)
@@ -119,25 +117,49 @@ class Puppet::Rails::Host < ActiveRecord::Base
     def setresources(list)
         existing = nil
         seconds = Benchmark.realtime {
+            existing = find_resources()
+        }
+        Puppet.notice("Searched for resources in %0.2f seconds" % seconds) if defined?(Puppet::TIME_DEBUG)
 
-            # Preload the parameters with the resource query, but not the tags, since doing so makes the query take about 10x longer.
-            # I've left the other queries in so that it's straightforward to switch between them for testing, if we so desire.
-            #existing = resources.find(:all, :include => [{:param_values => :param_name, :resource_tags => :puppet_tag}, :source_file]).inject({}) do | hash, resource |
-            #existing = resources.find(:all, :include => [{:resource_tags => :puppet_tag}, :source_file]).inject({}) do | hash, resource |
-            existing = resources.find(:all, :include => [{:param_values => :param_name}, :source_file]).inject({}) do | hash, resource |
-                hash[resource.ref] = resource
-                hash
-            end
+        seconds = Benchmark.realtime {
+            find_resources_parameters_tags(existing)
+        } if id
+        Puppet.notice("Searched for resource params and tags in %0.2f seconds" % seconds) if defined?(Puppet::TIME_DEBUG)
+
+        seconds = Benchmark.realtime {
+            compare_to_catalog(existing, list)
         }
+        Puppet.notice("Resource comparison took %0.2f seconds" % seconds) if defined?(Puppet::TIME_DEBUG)
+    end
 
-        Puppet.notice("Searched for resources in %0.2f seconds" % seconds) if defined?(Puppet::TIME_DEBUG)
+    def find_resources
+        resources.find(:all, :include => :source_file).inject({}) do | hash, resource |
+            hash[resource.ref] = resource
+            hash
+        end
+    end
+
+    def find_resources_parameters_tags(resources)
+        # initialize all resource parameters
+        resources.each do |key,resource|
+            resource.params_hash = []
+        end
 
+        resources_by_id = resources.inject({}) do |hash, res|
+            hash[res[1]['id']] = res[1]
+            hash
+        end
+
+        find_resources_parameters(resources_by_id)
+        find_resources_tags(resources_by_id)
+    end
+
+    def compare_to_catalog(resources, list)
         compiled = list.inject({}) do |hash, resource|
             hash[resource.ref] = resource
             hash
         end
-        
-        ar_hash_merge(existing, compiled,
+        ar_hash_merge(resources, compiled,
                       :create => Proc.new { |ref, resource|
                           resource.to_rails(self)
                       }, :delete => Proc.new { |resource|
@@ -147,6 +169,23 @@ class Puppet::Rails::Host < ActiveRecord::Base
                       })
     end
 
+    def find_resources_parameters(resources)
+        params = Puppet::Rails::ParamValue.find_all_params_from_host(self)
+
+        # assign each loaded parameters/tags to the resource it belongs to
+        params.each do |param|
+            resources[param['resource_id']].add_param_to_hash(param)
+        end
+    end
+
+    def find_resources_tags(resources)
+        tags = Puppet::Rails::ResourceTag.find_all_tags_from_host(self)
+
+        tags.each do |tag|
+            resources[tag['resource_id']].add_tag_to_hash(tag)
+        end
+    end
+
     def update_connect_time
         self.last_connect = Time.now
         save
diff --git a/lib/puppet/rails/resource.rb b/lib/puppet/rails/resource.rb
index 255b0e788..bd2739d53 100644
--- a/lib/puppet/rails/resource.rb
+++ b/lib/puppet/rails/resource.rb
@@ -5,6 +5,7 @@ require 'puppet/util/rails/collection_merger'
 
 class Puppet::Rails::Resource < ActiveRecord::Base
     include Puppet::Util::CollectionMerger
+    include Puppet::Util::ReferenceSerializer
 
     has_many :param_values, :dependent => :destroy, :class_name => "Puppet::Rails::ParamValue"
     has_many :param_names, :through => :param_values, :class_name => "Puppet::Rails::ParamName"
@@ -32,21 +33,46 @@ class Puppet::Rails::Resource < ActiveRecord::Base
         self.source_file = Puppet::Rails::SourceFile.find_or_create_by_filename(file)
     end
 
+    def title
+        unserialize_value(self[:title])
+    end
+
+    def add_param_to_hash(param)
+        @params_hash ||= []
+        @params_hash << param
+    end
+
+    def add_tag_to_hash(tag)
+        @tags_hash ||= []
+        @tags_hash << tag
+    end
+
+    def params_hash=(hash)
+        @params_hash = hash
+    end
+
+    def tags_hash=(hash)
+        @tags_hash = hash
+    end
+
     # returns a hash of param_names.name => [param_values]
     def get_params_hash(values = nil)
-        values ||= param_values.find(:all, :include => :param_name)
-        values.inject({}) do | hash, value |
-            hash[value.param_name.name] ||= []
-            hash[value.param_name.name] << value
+        values ||= @params_hash || Puppet::Rails::ParamValues.find_all_params_from_resource(id)
+        if values.size == 0
+            return {}
+        end
+        values.inject({}) do |hash, value|
+            hash[value['name']] ||= []
+            hash[value['name']] << value
             hash
         end
     end
-    
+
     def get_tag_hash(tags = nil)
-        tags ||= resource_tags.find(:all, :include => :puppet_tag)
+        tags ||= @tags_hash || Puppet::Rails::ResourceTag.find_all_tags_from_resource(id)
         return tags.inject({}) do |hash, tag|
             # We have to store the tag object, not just the tag name.
-            hash[tag.puppet_tag.name] = tag
+            hash[tag['name']] = tag
             hash
         end
     end
@@ -82,7 +108,7 @@ class Puppet::Rails::Resource < ActiveRecord::Base
     end
 
     def ref
-        "%s[%s]" % [self[:restype].split("::").collect { |s| s.capitalize }.join("::"), self[:title]]
+        "%s[%s]" % [self[:restype].split("::").collect { |s| s.capitalize }.join("::"), self.title.to_s]
     end
 
     # Convert our object to a resource.  Do not retain whether the object