Fixing #1903 - metaparam inheritance is much faster

This doesn't actually fix the specific request in #1903,
which said there should be no inheritance at all, but
I've changed my mind on that.  Static inheritance is good,
it should just be faster.

This change could result in up to 70% speed improvements
in compiling.

Signed-off-by: Luke Kanies <luke@puppetlabs.com>

4 files changed, 136 insertions, 76 deletions

diff --git a/lib/puppet/parser/compiler.rb b/lib/puppet/parser/compiler.rb
index fd7d9680b..ae4af7622 100644
--- a/lib/puppet/parser/compiler.rb
+++ b/lib/puppet/parser/compiler.rb
@@ -361,6 +361,50 @@ class Puppet::Parser::Compiler
 
             resource.finish if resource.respond_to?(:finish)
         end
+
+        add_resource_metaparams
+    end
+
+    def add_resource_metaparams
+        unless main = catalog.resource(:class, :main)
+            raise "Couldn't find main"
+        end
+
+        names = []
+        Puppet::Type.eachmetaparam do |name|
+            next if Puppet::Parser::Resource.relationship_parameter?(name)
+            names << name
+        end
+
+        data = {}
+        catalog.walk(main, :out) do |source, target|
+            if source_data = data[source] || metaparams_as_data(source, names)
+                # only store anything in the data hash if we've actually got
+                # data
+                data[source] ||= source_data
+                source_data.each do |param, value|
+                    target[param] = value if target[param].nil?
+                end
+                data[target] = source_data.merge(metaparams_as_data(target, names))
+            end
+
+            target.tag(*(source.tags))
+        end
+    end
+
+    def metaparams_as_data(resource, params)
+        data = nil
+        params.each do |param|
+            unless resource[param].nil?
+                # Because we could be creating a hash for every resource,
+                # and we actually probably don't often have any data here at all,
+                # we're optimizing a bit by only creating a hash if there's
+                # any data to put in it.
+                data ||= {}
+                data[param] = resource[param]
+            end
+        end
+        data
     end
 
     # Initialize the top-level scope, class, and resource.
diff --git a/lib/puppet/parser/resource.rb b/lib/puppet/parser/resource.rb
index 3e012247b..a4587723e 100644
--- a/lib/puppet/parser/resource.rb
+++ b/lib/puppet/parser/resource.rb
@@ -100,7 +100,6 @@ class Puppet::Parser::Resource < Puppet::Resource
         @finished = true
         add_defaults()
         add_metaparams()
-        add_scope_tags()
         validate()
     end
 
@@ -279,23 +278,8 @@ class Puppet::Parser::Resource < Puppet::Resource
         compat_mode = metaparam_compatibility_mode?
 
         Puppet::Type.eachmetaparam do |name|
-            if self.class.relationship_parameter?(name)
-                add_backward_compatible_relationship_param(name) if compat_mode
-                next
-            end
-
-            next if @parameters[name]
-
-            # Skip metaparams for which we get no value.
-            next unless val = scope.lookupvar(name.to_s, false) and val != :undefined
-
-            set_parameter(name, val)
-        end
-    end
-
-    def add_scope_tags
-        if scope_resource = scope.resource
-            tag(*scope_resource.tags)
+            next unless self.class.relationship_parameter?(name)
+            add_backward_compatible_relationship_param(name) if compat_mode
         end
     end
 
diff --git a/spec/unit/parser/compiler.rb b/spec/unit/parser/compiler.rb
index c36113ff6..d5d46b1f3 100755
--- a/spec/unit/parser/compiler.rb
+++ b/spec/unit/parser/compiler.rb
@@ -35,9 +35,10 @@ describe Puppet::Parser::Compiler do
         @node = Puppet::Node.new "testnode"
         @known_resource_types = Puppet::Resource::TypeCollection.new "development"
 
-        @scope_resource = stub 'scope_resource', :builtin? => true, :finish => nil, :ref => 'Class[main]', :type => "class"
-        @scope = stub 'scope', :resource => @scope_resource, :source => mock("source")
         @compiler = Puppet::Parser::Compiler.new(@node)
+        @scope = Puppet::Parser::Scope.new(:compiler => @compiler, :source => "fake_source")
+        @scope_resource = Puppet::Parser::Resource.new(:file, "/my/file", :scope => @scope)
+        @scope.resource = @scope_resource
         @compiler.environment.stubs(:known_resource_types).returns @known_resource_types
     end
 
@@ -249,35 +250,103 @@ describe Puppet::Parser::Compiler do
             @compiler.compile
         end
 
-        it "should call finish() on all resources" do
-            # Add a resource that does respond to :finish
-            resource = Puppet::Parser::Resource.new "file", "finish", :scope => @scope
-            resource.expects(:finish)
+        describe "when finishing" do
+            before do
+                @compiler.send(:evaluate_main)
+                @catalog = @compiler.catalog
+            end
 
-            @compiler.add_resource(@scope, resource)
+            def add_resource(name, parent = nil)
+                resource = Puppet::Parser::Resource.new "file", name, :scope => @scope
+                @compiler.add_resource(@scope, resource)
+                @catalog.add_edge(parent, resource) if parent
+                resource
+            end
 
-            # And one that does not
-            dnf = stub "dnf", :ref => "File[dnf]", :type => "file"
+            it "should call finish() on all resources" do
+                # Add a resource that does respond to :finish
+                resource = Puppet::Parser::Resource.new "file", "finish", :scope => @scope
+                resource.expects(:finish)
 
-            @compiler.add_resource(@scope, dnf)
+                @compiler.add_resource(@scope, resource)
 
-            @compiler.send(:finish)
-        end
+                # And one that does not
+                dnf = stub "dnf", :ref => "File[dnf]", :type => "file"
 
-        it "should call finish() in add_resource order" do
-            resources = sequence('resources')
+                @compiler.add_resource(@scope, dnf)
 
-            resource1 = Puppet::Parser::Resource.new "file", "finish1", :scope => @scope
-            resource1.expects(:finish).in_sequence(resources)
+                @compiler.send(:finish)
+            end
 
-            @compiler.add_resource(@scope, resource1)
+            it "should call finish() in add_resource order" do
+                resources = sequence('resources')
 
-            resource2 = Puppet::Parser::Resource.new "file", "finish2", :scope => @scope
-            resource2.expects(:finish).in_sequence(resources)
+                resource1 = add_resource("finish1")
+                resource1.expects(:finish).in_sequence(resources)
 
-            @compiler.add_resource(@scope, resource2)
+                resource2 = add_resource("finish2")
+                resource2.expects(:finish).in_sequence(resources)
+
+                @compiler.send(:finish)
+            end
+
+            it "should add each container's metaparams to its contained resources" do
+                main = @catalog.resource(:class, :main)
+                main[:noop] = true
+
+                resource1 = add_resource("meh", main)
+
+                @compiler.send(:finish)
+                resource1[:noop].should be_true
+            end
+
+            it "should add metaparams recursively" do
+                main = @catalog.resource(:class, :main)
+                main[:noop] = true
+
+                resource1 = add_resource("meh", main)
+                resource2 = add_resource("foo", resource1)
+
+                @compiler.send(:finish)
+                resource2[:noop].should be_true
+            end
+
+            it "should prefer metaparams from immediate parents" do
+                main = @catalog.resource(:class, :main)
+                main[:noop] = true
+
+                resource1 = add_resource("meh", main)
+                resource2 = add_resource("foo", resource1)
+
+                resource1[:noop] = false
+
+                @compiler.send(:finish)
+                resource2[:noop].should be_false
+            end
+
+            it "should merge tags downward" do
+                main = @catalog.resource(:class, :main)
+                main.tag("one")
+
+                resource1 = add_resource("meh", main)
+                resource1.tag "two"
+                resource2 = add_resource("foo", resource1)
+
+                @compiler.send(:finish)
+                resource2.tags.should be_include("one")
+                resource2.tags.should be_include("two")
+            end
+
+            it "should work if only middle resources have metaparams set" do
+                main = @catalog.resource(:class, :main)
+
+                resource1 = add_resource("meh", main)
+                resource1[:noop] = true
+                resource2 = add_resource("foo", resource1)
 
-            @compiler.send(:finish)
+                @compiler.send(:finish)
+                resource2[:noop].should be_true
+            end
         end
 
         it "should return added resources in add order" do
diff --git a/spec/unit/parser/resource.rb b/spec/unit/parser/resource.rb
index 5b2a16ac0..9d407c0e7 100755
--- a/spec/unit/parser/resource.rb
+++ b/spec/unit/parser/resource.rb
@@ -219,23 +219,6 @@ describe Puppet::Parser::Resource do
             @resource.should_not be_metaparam_compatibility_mode
         end
 
-        it "should copy metaparams from its scope" do
-            @scope.setvar("noop", "true")
-
-            @resource.class.publicize_methods(:add_metaparams)  { @resource.add_metaparams }
-
-            @resource["noop"].should == "true"
-        end
-
-        it "should not copy metaparams that it already has" do
-            @resource.set_parameter("noop", "false")
-            @scope.setvar("noop", "true")
-
-            @resource.class.publicize_methods(:add_metaparams)  { @resource.add_metaparams }
-
-            @resource["noop"].should == "false"
-        end
-
         it "should not copy relationship metaparams when not in metaparam compatibility mode" do
             @scope.setvar("require", "bar")
 
@@ -263,26 +246,6 @@ describe Puppet::Parser::Resource do
 
             @resource["require"].should == ["foo", "bar"]
         end
-
-        it "should copy all metaparams that it finds" do
-            @scope.setvar("noop", "foo")
-            @scope.setvar("schedule", "bar")
-
-            @resource.class.publicize_methods(:add_metaparams)  { @resource.add_metaparams }
-
-            @resource["noop"].should == "foo"
-            @resource["schedule"].should == "bar"
-        end
-
-        it "should add any tags from the scope resource" do
-            scope_resource = stub 'scope_resource', :tags => %w{one two}
-            @scope.stubs(:resource).returns(scope_resource)
-
-            @resource.class.publicize_methods(:add_scope_tags)  { @resource.add_scope_tags }
-
-            @resource.tags.should be_include("one")
-            @resource.tags.should be_include("two")
-        end
     end
 
     describe "when being tagged" do