Performance improvements #124
Conversation
|
The implied question was: is this a good path forward? If so, I can take a look at finishing them up. |
|
Great work @ekohl !!
I read this as a safe way to improve existing code. +1
I'm understanding this as we can currently test for a specific version of Facter and after this patch, we cannot. Very likely I'm totally misunderstanding this, though if that is the case, I would not want to do that or would want an option where you can bypass it and specifically test a given version of Facter.
I read this as another optimization so +1. I'm not sure what the ordering that potentially causing test failures means and what the implications are for that. |
|
Good idea using the nginx module for testing. Would suggest before merging to pick a handful of popular VP modules that are in a good state already and see if this has any unintended consequences. |
It should still work like that (since The queried facterversion is 3.1.2 but it expected 3.1.6 to be returned. After the second patch it returns a set of facts with version 3.0.2 since 3.1.6 > 3.1.2. I wonder if this is silently converted to a float in the filterspec by JGrep. Given there was an explicit test for it, that makes me think it's a regression. |
|
If you specify 3.1.2, I would expect it to respond with the same 3.1.2 or an error. |
|
You get the error if you specify strict versions, but that's not the default behavior. |
|
@sanfrancrisko at this point I'm uncertain what is expected. I'd like some guidance from the maintainers what the behavior should be. That means either changing the tests (as I suggested) or changing the code to pass the tests. Until I get that feedback I'm blocked. |
ekohl
force-pushed
the
performance-improvements
branch
from
4d80729
to
dac3701
Compare
This assumes that no exact match can be found in most cases. This saves a call to Facterdb. Some testing on puppet-nginx this reduced the time for a dry run by about 1 to 2 seconds, from about ~11 to ~9 seconds. Actual results will vary on the FacterDB. Note that a new Facter version will trigger the bad code unless all operating systems are updated so this is a very likely code path. It also makes the easier to read.
Rather than getting the facts for every filter spec and then discarding that, this stores the found facts in an array and uses that. This eliminates a call to FacterDB::get_facts with a very complex filter. A quick test in puppet-nginx reduces loading of tests by about 2 seconds (from ~9 to ~7).
ekohl
force-pushed
the
performance-improvements
branch
from
dac3701
to
0c9397f
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #124 +/- ##
==========================================
- Coverage 94.93% 94.87% -0.07%
==========================================
Files 2 2
Lines 158 156 -2
==========================================
- Hits 150 148 -2
Misses 8 8
☔ View full report in Codecov by Sentry. |
|
All work is merged, except #152. |
This includes some performance improvements that reduce the loading of tests by about 4 seconds (from ~11 to ~7 seconds) so about a third.
There are some gotchas. Each commit captures what it does. Copying for readability.
f39009f Move facterversion_obj declaration out of the loop
There's a very good chance we'll need this object and this saves parsing it over and over.
This is a safe patch.
5452fe9 Do not query for the exact facter version
This assumes that no exact match can be found in most cases. This saves a call to Facterdb. Some testing on puppet-nginx this reduced the time for a dry run by about 1 to 2 seconds, from about ~11 to ~9 seconds.
Actual results will vary on the FacterDB. Note that a new Facter version will trigger the bad code unless all operating systems are updated so this is a very likely code path.
It also makes the easier to read.
This appears to be backwards incompatible. I didn't expect it, but it looks like you can query for 3.1.2 and get back 3.1.6 in the current releases while I assumed that was impossible.
4d80729 Collect facts iteratively
Rather than getting the facts for every filter spec and then discarding that, this stores the found facts in an array and uses that.
This eliminates a call to FacterDB::get_facts with a very complex filter.
A quick test in puppet-nginx reduces loading of tests by about 2 seconds (from ~9 to ~7).
This should be fairly safe. The order of the results is now likely to be different and perhaps that's causing test failures.
Fixes #123