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

Refactor facts to improve performance: #375

Merged
merged 1 commit into from Oct 16, 2014
Merged

Refactor facts to improve performance: #375

merged 1 commit into from Oct 16, 2014

Conversation

raphink
Copy link

@raphink raphink commented Oct 15, 2014

  • Add an apt_has_updates boolean fact
  • Make other facts depend on it

These facts call the apt-check command 3 times for every run. This command usually takes 2 seconds or more to return, thus taking 6 seconds or more just for these 3 facts.

This PR proposes a refactoring which runs the command only once when no packages need to be updated, and twice if packages need updating (to get the package names).

I am well aware that it puts global variables outside of setcode blocks, but it adds a great performance gain.

Note: I refactored the spec tests because they were stubbing stuff blindly so most of them were passing by pure luck.

* Add an apt_has_updates boolean fact
* Make other facts depend on it
@daenney
Copy link

daenney commented Oct 15, 2014

I'm not adverse to the globals, a 4s speedup is a good enough reason. I'll try this out for a sec, see if I can replicate the performance issues.

@daenney
Copy link

daenney commented Oct 15, 2014

If I just run time /usr/lib/update-notifier/apt-check -p 2>&1 the worst speed I get is 0.6s. Where did you get your 2s+ from?

@raphink
Copy link
Author

raphink commented Oct 15, 2014

@daenney On Ubuntu laptops here (even using an SSD), this is what I get:

# time /usr/lib/update-notifier/apt-check -p 2>&1

real    0m2.008s
user    0m1.934s
sys     0m0.062s

@daenney
Copy link

daenney commented Oct 15, 2014

Uhg okay, that's a problem

@raphink
Copy link
Author

raphink commented Oct 15, 2014

I'm probably not the only one in this situation, so I'd rather fix this in the facts than debugging apt-check.

@daenney
Copy link

daenney commented Oct 15, 2014

Oh totally. Besides, by the time apt-check updates have made it to every platform and release involved we'll be twenty years older.

daenney added a commit that referenced this pull request Oct 16, 2014
Refactor facts to improve performance.
@daenney daenney merged commit 5d96da0 into puppetlabs:master Oct 16, 2014
@daenney
Copy link

daenney commented Oct 16, 2014

You know, from what I can see this looks good. It works and the tests are better. Plus I can really appreciate not calling out to the same command a bunch of times but 'caching' that response.

@LukasAud LukasAud added the bugfix label Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants