Implement autoloading #289
Conversation
Autoloading prevents having to think about what to exactly require. It can also postpone loading of classes until they're actually used. This can help with with application startup speed.
By only loading it when needed, loading speed can be improved.
Why is this a good thing? Like Python, I think it's beneficial for developers to be able to see what is required within a file to be able to trace where functionality is coming from. |
With Ruby it doesn't really work that way. For example, Python has a fixed way how the import path works. With Ruby you can require any path and it can have a completely different content. This was a bit inspired by Rails which uses autoloading (even though it's really a core Ruby feature). See https://edgeguides.rubyonrails.org/autoloading_and_reloading_constants.html as well. Note Foreman is not yet using Zeitwerk and is still on the classic loader. My major annoyance is that you constantly need to be aware of what's loaded. This means a ton of duplication in tests. If you're loading the entire application, it can also slow things down. That can be seen in Rake tasks which load the world and then some. Some unrepresentative testing. Without autoloading: $ time be ./bin/foreman-installer --list-scenarios
/home/ekohl/.gem/ruby/gems/highline-1.7.10/lib/highline.rb:624: warning: Using the last argument as keyword parameters is deprecated
Available scenarios
/home/ekohl/.gem/ruby/gems/highline-1.7.10/lib/highline.rb:624: warning: Using the last argument as keyword parameters is deprecated
Foreman Proxy Content /home/ekohl/.gem/ruby/gems/highline-1.7.10/lib/highline.rb:624: warning: Using the last argument as keyword parameters is deprecated
(use: --scenario foreman-proxy-content)
/home/ekohl/.gem/ruby/gems/highline-1.7.10/lib/highline.rb:624: warning: Using the last argument as keyword parameters is deprecated
Install a stand-alone Foreman Content Proxy.
/home/ekohl/.gem/ruby/gems/highline-1.7.10/lib/highline.rb:624: warning: Using the last argument as keyword parameters is deprecated
Katello /home/ekohl/.gem/ruby/gems/highline-1.7.10/lib/highline.rb:624: warning: Using the last argument as keyword parameters is deprecated
(use: --scenario katello)
/home/ekohl/.gem/ruby/gems/highline-1.7.10/lib/highline.rb:624: warning: Using the last argument as keyword parameters is deprecated
Install Foreman with Katello
/home/ekohl/.gem/ruby/gems/highline-1.7.10/lib/highline.rb:624: warning: Using the last argument as keyword parameters is deprecated
Foreman /home/ekohl/.gem/ruby/gems/highline-1.7.10/lib/highline.rb:624: warning: Using the last argument as keyword parameters is deprecated
(use: --scenario foreman)
/home/ekohl/.gem/ruby/gems/highline-1.7.10/lib/highline.rb:624: warning: Using the last argument as keyword parameters is deprecated
Default installation of Foreman
real 0m0.420s
user 0m0.361s
sys 0m0.056sWith autoloading: $ time be ./bin/foreman-installer --list-scenarios
/home/ekohl/.gem/ruby/gems/highline-1.7.10/lib/highline.rb:624: warning: Using the last argument as keyword parameters is deprecated
Available scenarios
/home/ekohl/.gem/ruby/gems/highline-1.7.10/lib/highline.rb:624: warning: Using the last argument as keyword parameters is deprecated
Foreman Proxy Content /home/ekohl/.gem/ruby/gems/highline-1.7.10/lib/highline.rb:624: warning: Using the last argument as keyword parameters is deprecated
(use: --scenario foreman-proxy-content)
/home/ekohl/.gem/ruby/gems/highline-1.7.10/lib/highline.rb:624: warning: Using the last argument as keyword parameters is deprecated
Install a stand-alone Foreman Content Proxy.
/home/ekohl/.gem/ruby/gems/highline-1.7.10/lib/highline.rb:624: warning: Using the last argument as keyword parameters is deprecated
Katello /home/ekohl/.gem/ruby/gems/highline-1.7.10/lib/highline.rb:624: warning: Using the last argument as keyword parameters is deprecated
(use: --scenario katello)
/home/ekohl/.gem/ruby/gems/highline-1.7.10/lib/highline.rb:624: warning: Using the last argument as keyword parameters is deprecated
Install Foreman with Katello
/home/ekohl/.gem/ruby/gems/highline-1.7.10/lib/highline.rb:624: warning: Using the last argument as keyword parameters is deprecated
Foreman /home/ekohl/.gem/ruby/gems/highline-1.7.10/lib/highline.rb:624: warning: Using the last argument as keyword parameters is deprecated
(use: --scenario foreman)
/home/ekohl/.gem/ruby/gems/highline-1.7.10/lib/highline.rb:624: warning: Using the last argument as keyword parameters is deprecated
Default installation of Foreman
real 0m0.381s
user 0m0.320s
sys 0m0.058sSo roughly a 10% improvement in start up time when you only want to list scenarios. Granted, it's all already sub-second, but the bigger the program becomes, the more you'll see it helps. For example, #290 was one where I think that adding a class file and the autoloader definition is easier than thinking about where to add it in the loading. I also ran into similar problems with the Smart Proxy. |
| @@ -446,6 +419,7 @@ def run_installation | |||
| log_parser = PuppetLogParser.new | |||
| logger = Logger.new('configure') | |||
|
|
|||
| require 'pty' | |||
There was a problem hiding this comment.
I dislike these mid-code requires. They can hide errors, and lead to missing libraries at runtime.
|
If I take #290 as the example, I can see that you introduced PuppetReport here -- https://github.com/theforeman/kafo/pull/290/files#diff-ef4f6d9e6e9975162ff8026e770e8787730a3547080b75534b8bdb411ff6efc2R478 And that by requiring that class here -- https://github.com/theforeman/kafo/pull/290/files#diff-ef4f6d9e6e9975162ff8026e770e8787730a3547080b75534b8bdb411ff6efc2R25 I now am able to trace where PuppetReport is coming from rather than having to guess if its from the code base itself, or some third party gem. I have been frustrated by this in the Ruby world many times. And Rails is the worst offender in this case. |
|
This gem follows the convention that everything is in lib and then convert CamelCase into snake_case. So |
| autoload :SystemChecker, 'kafo/system_checker' | ||
| autoload :TypeError, 'kafo/exceptions' | ||
| autoload :Wizard, 'kafo/wizard' | ||
| end |
There was a problem hiding this comment.
This seems overly verbose compared to something more automatic -- https://github.com/theforeman/tool_belt/blob/master/lib/tool_belt.rb#L2
There was a problem hiding this comment.
The difference is that tool_belt is loading it all up front where autoload is lazy loading. You can also copy the pattern used by autoloaded to automatically convert file names to class/module names.
Can this convention be enforced? |
Autoloading prevents having to think about what to exactly require. It can also postpone loading of classes until they're actually used. This can help with with application startup speed.
I only did minimal testing on this, but I think it makes the code cleaner. I considered autoloaded but wanted to avoid an additional dependency.