My inclination is to err on the side of removing caching and other efficiency hacks for the sake of simplifying code, build more benchmarking tools, and use benchmark metrics to guide further work on efficiency. This one, for example, looks to me like the multiple calls might get fixed at another level of abstraction and be even more efficient.
~Jesse On Tue, May 25, 2010 at 8:49 AM, Luke Kanies <[email protected]> wrote: > Hmm. > > We added the named_file_is_missing stuff because load was being called > massive numbers of times for files that weren't there, especially when > trying to autoload from modules. > > It looks like there was a later refactor that made this method only called > when an exception was raised, so it's probably ok that this is removed - at > least I can't find some reason that would indicate it's not. > > These performance refactors are tough to deal with down the road - it's > difficult to repeat them, therefore it's difficult to know if a later > refactor will bring them back, but I think this one is ok. > > So, in the end: +1 > > > On May 24, 2010, at 11:00 PM, Jesse Wolfe wrote: > > Change Autoloader's load to re-raise exceptions that happen when >> trying to load files, rather than just warning. >> >> This version still does not raise an error if the file is not found, as >> doing so would change the behavior of 'load' pretty significantly, but I >> am ambivalent this. >> >> Signed-off-by: Jesse Wolfe <[email protected]> >> --- >> lib/puppet/util/autoload.rb | 7 ++----- >> lib/puppet/util/autoload/file_cache.rb | 14 -------------- >> spec/unit/util/autoload.rb | 17 ++++------------- >> 3 files changed, 6 insertions(+), 32 deletions(-) >> >> diff --git a/lib/puppet/util/autoload.rb b/lib/puppet/util/autoload.rb >> index c7bf8ea..4a5de50 100644 >> --- a/lib/puppet/util/autoload.rb >> +++ b/lib/puppet/util/autoload.rb >> @@ -74,8 +74,6 @@ class Puppet::Util::Autoload >> # Load a single plugin by name. We use 'load' here so we can reload a >> # given plugin. >> def load(name) >> - return false if named_file_missing?(name) >> - >> path = name.to_s + ".rb" >> >> searchpath.each do |dir| >> @@ -90,11 +88,10 @@ class Puppet::Util::Autoload >> raise >> rescue Exception => detail >> puts detail.backtrace if Puppet[:trace] >> - warn "Could not autoload #{name}: #{detail}" >> - return named_file_is_missing(name) >> + raise Puppet::Error, "Could not autoload #{name}: >> #{detail}" >> end >> end >> - return named_file_is_missing(name) >> + false >> end >> >> # Mark the named object as loaded. Note that this supports unqualified >> diff --git a/lib/puppet/util/autoload/file_cache.rb >> b/lib/puppet/util/autoload/file_cache.rb >> index 4ad0c7f..881e086 100644 >> --- a/lib/puppet/util/autoload/file_cache.rb >> +++ b/lib/puppet/util/autoload/file_cache.rb >> @@ -73,20 +73,6 @@ module Puppet::Util::Autoload::FileCache >> missing_files[path] = Time.now >> end >> >> - def named_file_missing?(name) >> - @named_files ||= {} >> - if time = @named_files[name] and ! data_expired?(time) >> - return true >> - end >> - false >> - end >> - >> - def named_file_is_missing(name) >> - @named_files ||= {} >> - @named_files[name] = Time.now >> - false >> - end >> - >> private >> >> def cached_data?(path, type = nil) >> diff --git a/spec/unit/util/autoload.rb b/spec/unit/util/autoload.rb >> index f254f4f..9dc66f3 100755 >> --- a/spec/unit/util/autoload.rb >> +++ b/spec/unit/util/autoload.rb >> @@ -76,26 +76,17 @@ describe Puppet::Util::Autoload do >> end >> >> [RuntimeError, LoadError, SyntaxError].each do |error| >> - it "should not die an if a #{error.to_s} exception is thrown" >> do >> + it "should die with Puppet::Error if a #{error.to_s} >> exception is thrown" do >> @autoload.stubs(:file_exist?).returns true >> >> Kernel.expects(:load).raises error >> >> - @autoload.load("foo") >> + lambda { @autoload.load("foo") }.should >> raise_error(Puppet::Error) >> end >> end >> >> - it "should skip files that it knows are missing" do >> - @autoload.expects(:named_file_missing?).with("foo").returns >> true >> - @autoload.expects(:eachdir).never >> - >> - @autoload.load("foo") >> - end >> - >> - it "should register that files are missing if they cannot be >> found" do >> - @autoload.load("foo") >> - >> - @autoload.should be_named_file_missing("foo") >> + it "should not raise an error if the file is missing" do >> + @autoload.load("foo").should == false >> end >> >> it "should register loaded files with the main loaded file list so >> they are not reloaded by ruby" do >> -- >> 1.7.0.4 >> >> -- >> 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]<puppet-dev%[email protected]> >> . >> For more options, visit this group at >> http://groups.google.com/group/puppet-dev?hl=en. >> >> > > -- > Work is not always required. There is such a thing as sacred idleness. > -- George MacDonald > --------------------------------------------------------------------- > Luke Kanies -|- http://puppetlabs.com -|- +1(615)594-8199 > > > -- > 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]<puppet-dev%[email protected]> > . > For more options, visit this group at > http://groups.google.com/group/puppet-dev?hl=en. > > -- 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.
