Merge lp:~kissiel/checkbox/speed-optimization into lp:checkbox

[Maciej Kisielewski]

This MR is the first (and simple) patch set that improves plainbox startup time.

The speed-up depends on how many jobs there are in your provider 'database'.

Here are benchmarks from my slow NUC:

1000 jobs - 44s -> 24s
2500 jobs - 2m43s -> 1m19s

The benchmarks were done using this tool:
https://code.launchpad.net/~kissiel/+git/benchcbox/+ref/master

The tool generates providers and checkbox-cli launcher that doesn't run any of the jobs. Measuring wall time of running this launcher gives an estimate how long checkbox takes to 'spool up'.

[Paul Larson (pwlars)]

Wow, part of me really wants to pick this apart and find if there are even more gains, compare it to normal sets, and things like that, but I see no reason to block such an obvious improvement for any time at all. It's hard to argue with those results, this is great!

review: Approve

[Po-Hsu Lin (cypressyew)]

Cool stuff, thanks.

review: Approve

[Sylvain Pineau (sylvain-pineau)]

The attempt to merge lp:~kissiel/checkbox/speed-optimization into lp:checkbox failed. Below is the output from the failed tests.

[precise] starting container
[precise] (timing) 0.11user 0.03system 0:05.50elapsed 2%CPU (0avgtext+0avgdata 9960maxresident)k
[precise] (timing) 0inputs+32outputs (0major+6827minor)pagefaults 0swaps
[precise] provisioning container
[precise] (timing) 45.51user 18.60system 1:07.10elapsed 95%CPU (0avgtext+0avgdata 97156maxresident)k
[precise] (timing) 0inputs+19168outputs (0major+3199615minor)pagefaults 0swaps
[precise-testing] Starting tests...
Found a test script: ./checkbox-ng/requirements/container-tests-checkbox-ng-unit
[precise-testing] container-tests-checkbox-ng-unit: FAIL
[precise-testing] stdout: http://paste.ubuntu.com/23278019/
[precise-testing] stderr: http://paste.ubuntu.com/23278020/
[precise-testing] (timing) Command exited with non-zero status 1
[precise-testing] (timing) 0.69user 0.14system 0:00.85elapsed 98%CPU (0avgtext+0avgdata 48608maxresident)k
[precise-testing] (timing) 0inputs+1280outputs (0major+22164minor)pagefaults 0swaps
Found a test script: ./checkbox-support/requirements/container-tests-checkbox-support
[precise-testing] container-tests-checkbox-support: PASS
[precise-testing] (timing) 31.22user 0.35system 0:31.61elapsed 99%CPU (0avgtext+0avgdata 150684maxresident)k
[precise-testing] (timing) 0inputs+1376outputs (0major+39712minor)pagefaults 0swaps
Found a test script: ./converged/requirements/container-tests-touch-unit-tests
[precise-testing] container-tests-touch-unit-tests: PASS
[precise-testing] (timing) 0.00user 0.02system 0:00.02elapsed 86%CPU (0avgtext+0avgdata 2232maxresident)k
[precise-testing] (timing) 0inputs+8outputs (0major+2345minor)pagefaults 0swaps
Found a test script: ./plainbox/plainbox/impl/providers/categories/requirements/container-tests-provider-categories
[precise-testing] container-tests-provider-categories: PASS
[precise-testing] (timing) 1.05user 0.09system 0:01.15elapsed 99%CPU (0avgtext+0avgdata 47200maxresident)k
[precise-testing] (timing) 0inputs+64outputs (0major+14466minor)pagefaults 0swaps
Found a test script: ./plainbox/requirements/001-container-tests-plainbox-egg-info
[precise-testing] 001-container-tests-plainbox-egg-info: PASS
[precise-testing] (timing) 0.22user 0.05system 0:00.28elapsed 98%CPU (0avgtext+0avgdata 14568maxresident)k
[precise-testing] (timing) 0inputs+88outputs (0major+6046minor)pagefaults 0swaps
Found a test script: ./plainbox/requirements/container-tests-plainbox
[precise-testing] container-tests-plainbox: PASS
[precise-testing] (timing) 38.72user 1.39system 0:40.26elapsed 99%CPU (0avgtext+0avgdata 201576maxresident)k
[precise-testing] (timing) 0inputs+3304outputs (0major+263264minor)pagefaults 0swaps
Found a test script: ./plainbox/requirements/container-tests-plainbox-documentation
[precise-testing] container-tests-plainbox-documentation: PASS
[precise-testing] (timing) 102.37user 0.59system 1:43.22elapsed 99%CPU (0avgtext+0avgdata 185420maxresident)k
[precise-testing] (timing) 0inputs+43216outputs (0major+58278minor)pagefaults 0swaps...

[lp:~kissiel/checkbox/speed-optimization]

4520. By Maciej Kisielewski on 2016-10-05

checkbox-ng:misc: use expanded tuple for comparison

This way we check for 'not None' instead of going through full __eq__ chain.

Signed-off-by: Maciej Kisielewski <email address hidden>

[Maciej Kisielewski (kissiel)]

r.4520 fixes the failing test by unpacking (None, None) tuple and checking 'is not None' for both parts of the pair individually.

=== modified file 'checkbox-ng/checkbox_ng/misc.py'
--- checkbox-ng/checkbox_ng/misc.py	2016-03-11 13:46:41 +0000
+++ checkbox-ng/checkbox_ng/misc.py	2016-10-05 09:15:30 +0000
@@ -298,7 +298,7 @@
                 else:
                     tree.current_index += 1
                 result = category.get_node_by_index(index, tree)
-                if result != (None, None):
+                if result[0] is not None and result[1] is not None:
                     return result
             for job in self.jobs:
                 if index == tree.current_index:
 
=== modified file 'plainbox/plainbox/impl/session/state.py'
--- plainbox/plainbox/impl/session/state.py	2016-06-09 07:28:53 +0000
+++ plainbox/plainbox/impl/session/state.py	2016-10-05 09:15:30 +0000
@@ -413,7 +413,7 @@
 
         This method fires the :meth:`on_unit_added()` signal
         """
-        if unit in self._unit_list:
+        if unit in frozenset(self._unit_list):
             raise ValueError(
                 _("attempting to add the same unit twice: %s" % unit.id))
         self.state.add_unit(unit)
 
=== modified file 'plainbox/plainbox/impl/unit/test_unit.py'
--- plainbox/plainbox/impl/unit/test_unit.py	2016-05-20 13:54:59 +0000
+++ plainbox/plainbox/impl/unit/test_unit.py	2016-10-05 09:15:30 +0000
@@ -262,9 +262,6 @@
         self.assertNotEqual(
             Unit({}, parameters={'param': 'value1'}),
             Unit({}, parameters={'param': 'value2'}))
-        # Ensure that units are not equal to other classes
-        self.assertTrue(Unit({}) != object())
-        self.assertFalse(Unit({}) == object())
 
     def test_get_accessed_parameters(self):
         # There are no accessed parameters if the unit is not parameterized
 
=== modified file 'plainbox/plainbox/impl/unit/unit.py'
--- plainbox/plainbox/impl/unit/unit.py	2016-09-28 20:10:43 +0000
+++ plainbox/plainbox/impl/unit/unit.py	2016-10-05 09:15:30 +0000
@@ -383,6 +383,7 @@
         self._checksum = None
         self._parameters = parameters
         self._virtual = virtual
+        self._hash_cache = None
 
     @classmethod
     def instantiate_template(cls, data, raw_data, origin, provider, parameters,
@@ -405,8 +406,6 @@
                    field_offset_map)
 
     def __eq__(self, other):
-        if not isinstance(other, Unit):
-            return False
         return self.checksum == other.checksum
 
     def __ne__(self, other):
@@ -415,7 +414,9 @@
         return self.checksum != other.checksum
 
     def __hash__(self):
-        return hash(self.checksum)
+        if self._hash_cache is None:
+            self._hash_cache = int(self.checksum, 16)
+        return self._hash_cache
 
     @property
     def unit(self):