bugfix and simplification in TimeService

The main bug was that ceil was called on time returned by loop.time()
before being used to schedule the next iteration of the callback that
updated the TimeService.  The TimeService was then incrementing its
stored time by one second.  This resulted in the time being updated by
one second about every 1.5 seconds, quickly causing it to fall behind.

There was code to reset TimeService's internal time every 10 minutes,
but that has been removed.  Benchmarking has shown that just calling
time.time() has no statistically valid performance difference than the
code that checked for the need to reset.  Even if it were slightly
slower, it is only called once per second. Also, the counter that was
used to compare to the reset limit was never incremented, so the reset
never actually ever happened.

I was tempted to remove the properties for accessing time and loop time,
as time.time() and loop.time() are not expensive calls.  But I didn't
want to break the API for anyone using this.  The only significant
performance gain I was able to find was with strtime.  My use case
doesn't push aiohttp very hard, so I can't replicate whatever
performance issue TimeService was intended to solve.

Author: Greg Barnett

aiohttp/helpers.py

@@ -576,11 +576,11 @@ class TimeService:
     def __init__(self, loop, *, interval=1.0):
         self._loop = loop
         self._interval = interval
+
         self._time = time.time()
-        self._loop_time = loop.time()
-        self._count = 0
         self._strtime = None
-        self._cb = loop.call_at(self._loop_time + self._interval, self._on_cb)
+        self._loop_time = ceil(self._loop.time())
+        self._cb = self._loop.call_soon(self._on_cb)
 
     def close(self):
         if self._cb:
@@ -589,18 +589,12 @@ def close(self):
         self._cb = None
         self._loop = None
 
-    def _on_cb(self, reset_count=10*60):
-        if self._count >= reset_count:
-            # reset timer every 10 minutes
-            self._count = 0
-            self._time = time.time()
-        else:
-            self._time += self._interval
-
+    def _on_cb(self):
+        self._time = time.time()
         self._strtime = None
-        self._loop_time = ceil(self._loop.time())
-        self._cb = self._loop.call_at(
-            self._loop_time + self._interval, self._on_cb)
+        _loop_time = self._loop.time()
+        self._loop_time = ceil(_loop_time)
+        self._cb = self._loop.call_at(_loop_time + self._interval, self._on_cb)
 
     def _format_date_time(self):
         # Weekday and month names for HTTP date/time formatting;

tests/test_helpers.py

@@ -411,15 +411,31 @@ def test_strtime(self, time_service):
         # second call should use cached value
         assert time_service.strtime() == 'Sun, 30 Oct 2016 03:13:52 GMT'
 
-    def test_recalc_time(self, time_service, mocker):
+    def test_recalc_time(self, time_service):
         time = time_service._loop.time()
         time_service._time = time
         time_service._strtime = 'asd'
-        time_service._count = 1000000
         time_service._on_cb()
         assert time_service._strtime is None
         assert time_service._time > time
-        assert time_service._count == 0
+        assert time_service._loop_time > time
+
+    def test_on_cb_calls_on_cb_with_proper_delay(self, time_service):
+        mock_loop = mock.Mock()
+        mock_loop.time.side_effect = [1.1, 2.1]
+        mock_loop.call_at = mock.Mock()
+        time_service._loop = mock_loop
+        time_service._time = 0.5
+        time_service._interval = 1.0
+
+        start_time = time_service._time
+        time_service._on_cb()
+        mock_loop.call_at.assert_called_with(1.1 + time_service._interval, time_service._on_cb)
+        assert time_service._loop_time == 2
+
+        time_service._on_cb()
+        mock_loop.call_at.assert_called_with(2.1 + time_service._interval, time_service._on_cb)
+        assert time_service._loop_time == 3
 
 
 # ----------------------------------- TimeoutHandle -------------------