Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

slow custom facts #1273

Open
otheus opened this issue Oct 10, 2022 · 6 comments
Open

slow custom facts #1273

otheus opened this issue Oct 10, 2022 · 6 comments

Comments

@otheus
Copy link

otheus commented Oct 10, 2022

The package_provider.rb facts can be optimized simply by moving the require lines into the setcode block. This reduces stand-alone puppet runs by 0.5s on modern systems. (On puppet-agent runs, no such savings are observed, probably because the libraries are already required by the puppet agent itself. Apparently, the facter command evaluates the outer and inner blocks in different contexts.

Interestingly, moving the corresponding lines in service_provider.rb has no measurable performance benefit.

Current version

# bench 3 "facter -p >/dev/null"
Run 1
Run 2
Run 3
1665392723.165300291
1665392730.070952629
2.3018

With recommended change

# bench 3 "facter -p >/dev/null"
Run 1
Run 2
Run 3
1665392652.861524963
1665392661.744424272
2.9609
@otheus otheus changed the title package_provider fact slow slow custom facts Oct 10, 2022
@ekohl
Copy link
Collaborator

ekohl commented Oct 16, 2022

Could you come up with a patch?

@otheus
Copy link
Author

otheus commented Nov 17, 2022

@ekohl I'm really not interested in setting up an entire development environment to make these simple patches. In the past, I had to add test cases without having any idea how to run test cases. In our case, it's about 3 or 4 different files where those require statements simply need to be moved down.

@kenyon
Copy link
Contributor

kenyon commented Nov 17, 2022

All you need to do is fork and clone this repository, make the change, commit, push, create pull request.

@otheus
Copy link
Author

otheus commented Nov 18, 2022

Isn't that wishful thinking / bad process design? I do that, then someone says, "where are the test cases"? If I don't do that, who writes the test cases?

@ekohl
Copy link
Collaborator

ekohl commented Nov 18, 2022

For require statements it's hard to write tests so in this case I'm not sure what could actually be tested anyway.

Having said that: I can't reproduce your results.

First I try with a baseline: just facter.

$ bench 'facter'
benchmarking facter
time                 412.0 ms   (392.5 ms .. 452.3 ms)
                     0.999 R²   (0.998 R² .. 1.000 R²)
mean                 432.4 ms   (419.1 ms .. 452.2 ms)
std dev              19.44 ms   (4.462 ms .. 25.49 ms)
variance introduced by outliers: 19% (moderately inflated)

Then in a checkout of this repo (at e1977c5):

$ FACTERLIB=lib/facter bench 'facter -p'
benchmarking facter -p
time                 3.043 s    (3.020 s .. 3.058 s)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 3.010 s    (2.991 s .. 3.021 s)
std dev              18.18 ms   (7.728 ms .. 23.97 ms)
variance introduced by outliers: 19% (moderately inflated)

Then I move the code:

diff --git a/lib/facter/package_provider.rb b/lib/facter/package_provider.rb
index c5c1ef85..049fc997 100644
--- a/lib/facter/package_provider.rb
+++ b/lib/facter/package_provider.rb
@@ -9,13 +9,13 @@
 #
 # Caveats:
 #
-require 'puppet/type'
-require 'puppet/type/package'
 
 # These will be nil if Puppet is not available.
 Facter.add(:package_provider) do
   # Instantiates a dummy package resource and return the provider
   setcode do
+    require 'puppet/type'
+    require 'puppet/type/package'
     Puppet::Type.type(:package).newpackage(name: 'dummy', allow_virtual: 'true')[:provider].to_s
   end
 end

And again run the test:

$ FACTERLIB=lib/facter bench 'facter -p'
benchmarking facter -p
time                 3.108 s    (3.096 s .. 3.122 s)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 3.055 s    (3.010 s .. 3.075 s)
std dev              32.59 ms   (3.441 ms .. 41.48 ms)
variance introduced by outliers: 19% (moderately inflated)

From what I see it actually goes slightly up rather than down.

I tested this on Fedora 36, using Linux kernel 5.19.15-201.fc36.x86_64.

If you're not willing to submit a patch and I can't reproduce the benefit, should we then close this issue?

@otheus
Copy link
Author

otheus commented Nov 18, 2022

If you're not willing to submit a patch and I can't reproduce the benefit, should we then close this issue?

No, because I am required by my org to push local changes upstream, and the difference here is substantial and reproducible.

Our setup is significantly more complicated than your test scenario, but I imagine there are many variables.

I guess I will submit a patch. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants