+1 On 23/07/10 18:01, Markus Roberts wrote: > From: Brice Figureau <[email protected]> > > Each time the compiler was accessing the loaded types, we were checking if the > manifests had changed. This incurred a large performance cost compared to > 0.25 > and introduced race conditions if manifests changed while a thread was in the > middle of a compilation. > > This tentative fix, based on Brice's, makes sure each thread will get access > to > the same loaded types collection for the durration of a compilation, even if > the manifests change. We now only check for changed files at the start of a > compilation or if the environment changes, and we maintain a per environment > thread lock so that only one thread at a time can be reloading any particular > environment (and the need-check is done inside the synchronize block so that > only the first will actually load it). > > As long as the manifests don't change, the threads will share the same > collection, > so there is only duplication in memory for a brief window surrounding a > change. > > Signed-off-by: Brice Figureau <[email protected]> > Second-author: Markus Roberts <[email protected]> > Signed-off-by: Markus Roberts <[email protected]> > --- > lib/puppet/node/environment.rb | 20 +++++++++++++++----- > lib/puppet/parser/compiler.rb | 5 +++++ > spec/unit/node/environment_spec.rb | 35 ++++++++++++++++++++++++++++++++--- > 3 files changed, 52 insertions(+), 8 deletions(-) > > diff --git a/lib/puppet/node/environment.rb b/lib/puppet/node/environment.rb > index 762599c..3f67474 100644 > --- a/lib/puppet/node/environment.rb > +++ b/lib/puppet/node/environment.rb > @@ -1,4 +1,5 @@ > require 'puppet/util/cacher' > +require 'monitor' > > # Just define it, so this class has fewer load dependencies. > class Puppet::Node > @@ -67,14 +68,23 @@ class Puppet::Node::Environment > > def initialize(name) > @name = name > + extend MonitorMixin > end > > def known_resource_types > - if @known_resource_types.nil? or @known_resource_types.stale? > - @known_resource_types = Puppet::Resource::TypeCollection.new(self) > - @known_resource_types.perform_initial_import > - end > - @known_resource_types > + # This makes use of short circuit evaluation to get the right thread-safe > + # per environment semantics with an efficient most common cases; we > almost > + # always just return our thread's known-resource types. Only at the > start > + # of a compilation (after our thread var has been set to nil) or when the > + # environment has changed do we delve deeper. > + Thread.current[:known_resource_types] = nil if (krt = > Thread.current[:known_resource_types]) && krt.environment != self > + Thread.current[:known_resource_types] ||= synchronize { > + if @known_resource_types.nil? or @known_resource_types.stale? > + @known_resource_types = Puppet::Resource::TypeCollection.new(self) > + @known_resource_types.perform_initial_import > + end > + @known_resource_types > + } > end > > def module(name) > diff --git a/lib/puppet/parser/compiler.rb b/lib/puppet/parser/compiler.rb > index a901c0d..760d5a7 100644 > --- a/lib/puppet/parser/compiler.rb > +++ b/lib/puppet/parser/compiler.rb > @@ -15,6 +15,11 @@ class Puppet::Parser::Compiler > include Puppet::Resource::TypeCollectionHelper > > def self.compile(node) > + # At the start of a new compile we don't assume anything about > + # known_resouce_types; we'll get these from the environment and > + # cache them in a thread variable for the duration of the > + # compilation. > + Thread.current[:known_resource_types] = nil > new(node).compile.to_resource > rescue => detail > puts detail.backtrace if Puppet[:trace] > diff --git a/spec/unit/node/environment_spec.rb > b/spec/unit/node/environment_spec.rb > index b400865..6edcce5 100755 > --- a/spec/unit/node/environment_spec.rb > +++ b/spec/unit/node/environment_spec.rb > @@ -53,6 +53,7 @@ describe Puppet::Node::Environment do > @env = Puppet::Node::Environment.new("dev") > @collection = Puppet::Resource::TypeCollection.new(@env) > @collection.stubs(:perform_initial_import) > + Thread.current[:known_resource_types] = nil > end > > it "should create a resource type collection if none exists" do > @@ -71,13 +72,41 @@ describe Puppet::Node::Environment do > @env.known_resource_types > end > > - it "should create and return a new collection rather than returning a > stale collection" do > - @env.known_resource_types.expects(:stale?).returns true > + it "should return the same collection even if stale if it's the same > thread" do > + Puppet::Resource::TypeCollection.stubs(:new).returns @collection > + @env.known_resource_types.stubs(:stale?).returns true > > - Puppet::Resource::TypeCollection.expects(:new).returns @collection > + @env.known_resource_types.should equal(@collection) > + end > + > + it "should return the current thread associated collection if there is > one" do > + Thread.current[:known_resource_types] = @collection > > @env.known_resource_types.should equal(@collection) > end > + > + it "should give to all threads the same collection if it didn't change" > do > + Puppet::Resource::TypeCollection.expects(:new).with(@env).returns > @collection > + @env.known_resource_types > + > + t = Thread.new { > + @env.known_resource_types.should equal(@collection) > + } > + t.join > + end > + > + it "should give to new threads a new collection if it isn't stale" do > + Puppet::Resource::TypeCollection.expects(:new).with(@env).returns > @collection > + @env.known_resource_types.expects(:stale?).returns(true) > + > + Puppet::Resource::TypeCollection.expects(:new).returns @collection > + > + t = Thread.new { > + @env.known_resource_types.should equal(@collection) > + } > + t.join > + end > + > end > > [:modulepath, :manifestdir].each do |setting|
-- Brice Figureau My Blog: http://www.masterzen.fr/ -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
