Fixing a significant performance bug in file recursion, and trying to help performance a bit in attribute handling on types

git-svn-id: https://reductivelabs.com/svn/puppet/trunk@862 980ebf18-57e1-0310-9a29-db15c13687c0

10 files changed, 69 insertions, 62 deletions

diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index 306f6c277..92fd12423 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -538,11 +538,18 @@ class Type < Puppet::Element
 
     # Find the class associated with any given attribute.
     def self.attrclass(name)
-        case self.attrtype(name)
-        when :state: @validstates[name]
-        when :meta: @@metaparamhash[name]
-        when :param: @paramhash[name]
+        @attrclasses ||= {}
+
+        # We cache the value, since this method gets called such a huge number
+        # of times (as in, hundreds of thousands in a given run).
+        unless @attrclasses.include?(name)
+            @attrclasses[name] = case self.attrtype(name)
+            when :state: @validstates[name]
+            when :meta: @@metaparamhash[name]
+            when :param: @paramhash[name]
+            end
         end
+        @attrclasses[name]
     end
 
     def self.to_s
@@ -602,16 +609,22 @@ class Type < Puppet::Element
         end
     end
 
-    # What type of parameter are we dealing with?
+    # What type of parameter are we dealing with? Cache the results, because
+    # this method gets called so many times.
     def self.attrtype(name)
-        case
-        when @validstates.include?(name): return :state
-        when @@metaparamhash.include?(name): return :meta
-        when @paramhash.include?(name): return :param
-        else
-            raise Puppet::DevError, "Invalid attribute '%s' for class '%s'" %
-                [name, self.name]
+        @attrtypes ||= {}
+        unless @attrtypes.include?(name)
+            @attrtypes[name] = case
+                when @validstates.include?(name): :state
+                when @@metaparamhash.include?(name): :meta
+                when @paramhash.include?(name): :param
+                else
+                    raise Puppet::DevError, "Invalid attribute '%s' for class '%s'" %
+                        [name, self.name]
+                end
         end
+
+        @attrtypes[name]
     end
 
     # All parameters, in the appropriate order.  The namevar comes first,
@@ -619,12 +632,15 @@ class Type < Puppet::Element
     # were specified in the files.
     def self.allattrs
         # now get all of the arguments, in a specific order
-        order = [self.namevar]
+        # Cache this, since it gets called so many times
+        namevar = self.namevar
+
+        order = [namevar]
         order << [self.states.collect { |state| state.name },
             self.parameters,
             self.metaparams].flatten.reject { |param|
                 # we don't want our namevar in there multiple times
-                param == self.namevar
+                param == namevar
         }
 
         order.flatten!
@@ -1429,7 +1445,7 @@ class Type < Puppet::Element
 
         unless @name
             self.devfail "Could not find namevar '%s' for %s" %
-                [namevar, self.class.name]
+                [self.class.namevar, self.class.name]
         end
 
         return @name
diff --git a/lib/puppet/type/pfile.rb b/lib/puppet/type/pfile.rb
index 3010d51cf..16193578f 100644
--- a/lib/puppet/type/pfile.rb
+++ b/lib/puppet/type/pfile.rb
@@ -199,7 +199,9 @@ module Puppet
             super
         end
         
-        def newchild(path, hash = {})
+        # Create a new file or directory object as a child to the current
+        # object.
+        def newchild(path, local, hash = {})
             # make local copy of arguments
             args = @arghash.dup
 
@@ -257,14 +259,20 @@ module Puppet
                         path
                     return nil
                 end
-                args.each { |var,value|
-                    next if var == :path
-                    next if var == :name
-                    # behave idempotently
-                    unless child.should(var) == value
-                        child[var] = value
-                    end
-                }
+
+                # This is only necessary for sourcerecurse, because we might have
+                # created the object with different 'should' values than are
+                # set remotely.
+                unless local
+                    args.each { |var,value|
+                        next if var == :path
+                        next if var == :name
+                        # behave idempotently
+                        unless child.should(var) == value
+                            child[var] = value
+                        end
+                    }
+                end
             else # create it anew
                 #notice "Creating new file with args %s" % args.inspect
                 args[:parent] = self
@@ -367,11 +375,6 @@ module Puppet
                 return
             end
 
-            unless FileTest.directory? self.name
-                self.devfail(
-                    "Uh, somehow trying to manage non-dir %s" % self.name
-                )
-            end
             unless FileTest.readable? self.name
                 self.notice "Cannot manage %s: permission denied" % self.name
                 return
@@ -388,13 +391,11 @@ module Puppet
             children.each { |file|
                 file = File.basename(file)
                 next if file =~ /^\.\.?$/ # skip . and .. 
-                if child = self.newchild(file, :recurse => recurse)
+                if child = self.newchild(file, true, :recurse => recurse)
                     unless @children.include?(child)
                         self.push child
                         added.push file
-
                     end
-                    child.retrieve
                 end
             }
         end
@@ -447,7 +448,7 @@ module Puppet
                 if type == file
                     args[:recurse] = nil
                 end
-                self.newchild(name, args)
+                self.newchild(name, false, args)
                 #self.newchild(hash, source, recurse)
                 #hash2child(hash, source, recurse)
             }
diff --git a/lib/puppet/type/pfile/checksum.rb b/lib/puppet/type/pfile/checksum.rb
index 5d936f91e..eac454908 100755
--- a/lib/puppet/type/pfile/checksum.rb
+++ b/lib/puppet/type/pfile/checksum.rb
@@ -81,9 +81,11 @@ module Puppet
                     end
                 end
             when "timestamp","mtime":
-                sum = File.stat(@parent[:path]).mtime.to_s
+                sum = @parent.stat.mtime.to_s
+                #sum = File.stat(@parent[:path]).mtime.to_s
             when "time":
-                sum = File.stat(@parent[:path]).ctime.to_s
+                sum = @parent.stat.ctime.to_s
+                #sum = File.stat(@parent[:path]).ctime.to_s
             else
                 raise Puppet::Error, "Invalid sum type %s" % checktype
             end
diff --git a/lib/puppet/type/pfile/ensure.rb b/lib/puppet/type/pfile/ensure.rb
index ab5639937..33b6edf96 100755
--- a/lib/puppet/type/pfile/ensure.rb
+++ b/lib/puppet/type/pfile/ensure.rb
@@ -88,7 +88,7 @@ module Puppet
         end
 
         def retrieve
-            if stat = @parent.stat(true)
+            if stat = @parent.stat(false)
                 @is = stat.ftype.intern
             else
                 if self.should == :false
diff --git a/lib/puppet/type/pfile/group.rb b/lib/puppet/type/pfile/group.rb
index 8d401b722..56aaa07f7 100755
--- a/lib/puppet/type/pfile/group.rb
+++ b/lib/puppet/type/pfile/group.rb
@@ -29,7 +29,7 @@ module Puppet
         end
 
         def retrieve
-            stat = @parent.stat(true)
+            stat = @parent.stat(false)
 
             if stat
                 self.is = stat.gid
diff --git a/lib/puppet/type/pfile/mode.rb b/lib/puppet/type/pfile/mode.rb
index 31d1a264a..455d52ce1 100755
--- a/lib/puppet/type/pfile/mode.rb
+++ b/lib/puppet/type/pfile/mode.rb
@@ -75,7 +75,7 @@ module Puppet
         end
 
         def retrieve
-            if stat = @parent.stat(true)
+            if stat = @parent.stat(false)
                 self.is = stat.mode & 007777
                 unless defined? @fixed
                     if defined? @should and @should
diff --git a/lib/puppet/type/pfile/type.rb b/lib/puppet/type/pfile/type.rb
index 0ea548c26..e5db7d694 100755
--- a/lib/puppet/type/pfile/type.rb
+++ b/lib/puppet/type/pfile/type.rb
@@ -8,7 +8,7 @@ module Puppet
         #end
         
         def retrieve
-            if stat = @parent.stat(true)
+            if stat = @parent.stat(false)
                 @is = stat.ftype
             else
                 @is = :absent
diff --git a/lib/puppet/type/pfile/uid.rb b/lib/puppet/type/pfile/uid.rb
index 698915aff..006712c1b 100755
--- a/lib/puppet/type/pfile/uid.rb
+++ b/lib/puppet/type/pfile/uid.rb
@@ -80,7 +80,7 @@ module Puppet
         end
 
         def retrieve
-            unless stat = @parent.stat(true)
+            unless stat = @parent.stat(false)
                 @is = :absent
                 return
             end
diff --git a/test/types/file.rb b/test/types/file.rb
index a54b78e12..9b2b1653a 100644
--- a/test/types/file.rb
+++ b/test/types/file.rb
@@ -427,7 +427,7 @@ class TestFile < Test::Unit::TestCase
         }
 
         assert_nothing_raised {
-            dir.retrieve
+            dir.evaluate
         }
 
         subobj = nil
@@ -435,18 +435,18 @@ class TestFile < Test::Unit::TestCase
             subobj = Puppet.type(:file)[subdir]
         }
 
-        assert(subobj, "Could not retrieve subdir object")
+        assert(subobj, "Could not retrieve %s object" % subdir)
 
         File.open(tmpfile, "w") { |f| f.puts "yayness" }
 
-        dir.retrieve
+        dir.evaluate
 
         file = nil
         assert_nothing_raised {
             file = Puppet.type(:file)[tmpfile]
         }
 
-        assert(file, "Could not retrieve file object")
+        assert(file, "Could not retrieve %s object" % tmpfile)
 
     end
 
@@ -560,7 +560,7 @@ class TestFile < Test::Unit::TestCase
         assert_nil(obj, "Retrieved removed object")
     end
 
-    def test_zpath
+    def test_path
         dir = tempfile()
 
         path = File.join(dir, "and", "a", "sub", "dir")
@@ -581,7 +581,7 @@ class TestFile < Test::Unit::TestCase
         }
 
         assert_nothing_raised {
-            dirobj.retrieve
+            dirobj.evaluate
         }
 
         assert_nothing_raised {
diff --git a/test/types/filesources.rb b/test/types/filesources.rb
index e08fa268c..3d020d628 100755
--- a/test/types/filesources.rb
+++ b/test/types/filesources.rb
@@ -55,11 +55,11 @@ class TestFileSources < Test::Unit::TestCase
         }
         child = nil
         assert_nothing_raised {
-            child = file.newchild("childtest")
+            child = file.newchild("childtest", true)
         }
         assert(child)
         assert_raise(Puppet::DevError) {
-            file.newchild(File.join(path,"childtest"))
+            file.newchild(File.join(path,"childtest"), true)
         }
     end
 
@@ -114,18 +114,9 @@ class TestFileSources < Test::Unit::TestCase
                 "source" => fromdir
             )
         }
-        comp = Puppet.type(:component).create(
-            :name => "component"
-        )
-        comp.push tofile
-        assert_nothing_raised {
-            trans = comp.evaluate
-        }
-        assert_nothing_raised {
-            trans.evaluate
-        }
+        assert_apply(tofile)
 
-        assert(FileTest.exists?(todir))
+        assert(FileTest.exists?(todir), "Created dir %s does not exist" % todir)
         Puppet::Type.allclear
     end
 
@@ -369,9 +360,6 @@ class TestFileSources < Test::Unit::TestCase
         server = nil
         basedir = tempfile()
         @@tmpfiles << basedir
-        if File.exists?(basedir)
-            system("rm -rf %s" % basedir)
-        end
         Dir.mkdir(basedir)
 
         mounts = {