Bug 891907 - in talos, counter manager instantiation, management, and cleanup is not intuitive or well written. r=jmaher
authorJulien Pagès <j.parkouss@gmail.com>
Wed, 12 Aug 2015 16:15:47 +0200
changeset 992 7fe8298e0aa2
parent 991 e8854f96ade3
child 993 55711f8341c5
push id682
push userj.parkouss@gmail.com
push dateThu, 13 Aug 2015 05:30:41 +0000
reviewersjmaher
bugs891907
Bug 891907 - in talos, counter manager instantiation, management, and cleanup is not intuitive or well written. r=jmaher
talos/cmanager.py
talos/ttest.py
--- a/talos/cmanager.py
+++ b/talos/cmanager.py
@@ -1,12 +1,15 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
+import mozinfo
+import threading
+
 
 class CounterManager(object):
 
     counterDict = {}
 
     def __init__(self):
         self.allCounters = {}
         self.registeredCounters = {}
@@ -25,8 +28,55 @@ class CounterManager(object):
                 self.registeredCounters[counter] = \
                     [self.allCounters[counter], []]
 
     def getCounterValue(self, counterName):
         """Returns the last value of the counter 'counterName'"""
 
     def updatePidList(self):
         """Updates the list of PIDs we're interested in"""
+
+
+if mozinfo.os == 'linux':
+    from talos.cmanager_linux import LinuxCounterManager \
+        as DefaultCounterManager
+elif mozinfo.os == 'win':
+    from talos.cmanager_win32 import WinCounterManager \
+        as DefaultCounterManager
+else:  # mac
+    from talos.cmanager_mac import MacCounterManager \
+        as DefaultCounterManager
+
+
+class CounterManagement(object):
+    def __init__(self, process, counters, resolution):
+        """
+        Public interface to manage counters.
+
+        On creation, automatically spawn a thread to collect counters.
+        Be sure to call :meth:`stop` to stop the thread.
+        """
+        assert counters
+        self._manager = DefaultCounterManager(process, counters)
+        self._raw_counters = counters
+        self._counter_results = \
+            dict([(counter, []) for counter in self._raw_counters])
+
+        self._resolution = resolution
+        self._stop = threading.Event()
+        self._thread = threading.Thread(target=self._collect)
+        self._thread.start()
+
+    def _collect(self):
+        while not self._stop.wait(self._resolution):
+            # Get the output from all the possible counters
+            for count_type in self._raw_counters:
+                val = self._manager.getCounterValue(count_type)
+                if val:
+                    self._counter_results[count_type].append(val)
+
+    def stop(self):
+        self._stop.set()
+        self._thread.join()
+
+    def results(self):
+        assert not self._thread.is_alive()
+        return self._counter_results
--- a/talos/ttest.py
+++ b/talos/ttest.py
@@ -19,54 +19,39 @@ import traceback
 import subprocess
 import time
 import utils
 import mozcrash
 import talosconfig
 import shutil
 import mozfile
 import logging
-from threading import Thread
 
 from talos.utils import TalosError, TalosCrash, TalosRegression
 from talos import ffprocess, TalosProcess
 from talos.ffsetup import FFSetup
+from talos.cmanager import CounterManagement
 
 
 class TTest(object):
 
     _pids = []
-    platform_type = ''
-
-    def __init__(self):
-        cmanager, platformtype = self.getPlatformType()
-        self.CounterManager = cmanager
-        self.platform_type = platformtype
-
-    def getPlatformType(self):
-        if platform.system() == "Linux":
-            import cmanager_linux
-            CounterManager = cmanager_linux.LinuxCounterManager
-            platform_type = 'linux_'
-        elif platform.system() in ("Windows", "Microsoft"):
-            if '5.1' in platform.version():  # winxp
-                platform_type = 'win_'
-            elif '6.1' in platform.version():  # w7
-                platform_type = 'w7_'
-            elif '6.2' in platform.version():  # w8
-                platform_type = 'w8_'
-            else:
-                raise TalosError('unsupported windows version')
-            import cmanager_win32
-            CounterManager = cmanager_win32.WinCounterManager
-        elif platform.system() == "Darwin":
-            import cmanager_mac
-            CounterManager = cmanager_mac.MacCounterManager
-            platform_type = 'mac_'
-        return CounterManager, platform_type
+    if platform.system() == "Linux":
+        platform_type = 'linux_'
+    elif platform.system() in ("Windows", "Microsoft"):
+        if '5.1' in platform.version():  # winxp
+            platform_type = 'win_'
+        elif '6.1' in platform.version():  # w7
+            platform_type = 'w7_'
+        elif '6.2' in platform.version():  # w8
+            platform_type = 'w8_'
+        else:
+            raise TalosError('unsupported windows version')
+    elif platform.system() == "Darwin":
+        platform_type = 'mac_'
 
     def cleanupAndCheckForCrashes(self, browser_config, profile_dir,
                                   test_name):
         """cleanup browser processes and process crashes if found"""
 
         # cleanup processes
         ffprocess.cleanup_processes(self._pids,
                                     browser_config['browser_wait'])
@@ -96,17 +81,17 @@ class TTest(object):
                                            browser_config['symbols_path'],
                                            stackwalk_binary=stackwalkbin,
                                            test_name=test_name)
         mozfile.remove(minidumpdir)
 
         if found:
             raise TalosCrash("Found crashes after test run, terminating test")
 
-    def testCleanup(self, browser_config, profile_dir, test_config, cm):
+    def testCleanup(self, browser_config, profile_dir, test_config):
         try:
             if os.path.isfile(browser_config['browser_log']):
                 with open(browser_config['browser_log'], "r") as results_file:
                     results_raw = results_file.read()
                 logging.info(results_raw)
 
             if profile_dir:
                 try:
@@ -119,49 +104,36 @@ class TTest(object):
                     pass
 
         except TalosError, te:
             logging.debug("cleanup error: %s", te)
         except Exception:
             logging.debug("unknown error during cleanup: %s"
                           % (traceback.format_exc(),))
 
-    def collectCounters(self):
-        # set up the counters for this test
-        if self.counters:
-            while not self.isFinished:
-                time.sleep(self.resolution)
-                # Get the output from all the possible counters
-                for count_type in self.counters:
-                    if not self.cm:
-                        continue
-                    val = self.cm.getCounterValue(count_type)
-                    if val and self.counter_results:
-                        self.counter_results[count_type].append(val)
-
     def runTest(self, browser_config, test_config):
         """
             Runs an url based test on the browser as specified in the
             browser_config dictionary
 
         Args:
             browser_config:  Dictionary of configuration options for the
                              browser (paths, prefs, etc)
             test_config   :  Dictionary of configuration for the given
                              test (url, cycles, counters, etc)
 
         """
 
         logging.debug("operating with platform_type : %s", self.platform_type)
-        self.counters = test_config.get(self.platform_type + 'counters', [])
-        self.resolution = test_config['resolution']
         with FFSetup(browser_config, test_config) as setup:
             return self._runTest(browser_config, test_config, setup)
 
     def _runTest(self, browser_config, test_config, setup):
+        counters = test_config.get(self.platform_type + 'counters', [])
+        resolution = test_config['resolution']
         try:
 
             # add the mainthread_io to the environment variable, as defined
             # in test.py configs
             here = os.path.dirname(os.path.realpath(__file__))
             if test_config['mainthread']:
                 mainthread_io = os.path.join(here, "mainthread_io.log")
                 setup.env['MOZ_MAIN_THREAD_IO_LOG'] = mainthread_io
@@ -230,28 +202,26 @@ class TTest(object):
                     browser_config["browser_path"],
                     browser_config["extra_args"],
                     setup.profile_dir,
                     test_config['url'],
                     profiling_info=(setup.sps_profile.profiling_info
                                     if setup.sps_profile else None)
                 )
 
-                self.counter_results = None
                 mainthread_error_count = 0
                 if test_config['setup']:
                     # Generate bcontroller.json for xperf
                     talosconfig.generateTalosConfig(command_args,
                                                     browser_config,
                                                     test_config)
                     subprocess.call(
                         ['python'] + test_config['setup'].split(),
                     )
 
-                self.isFinished = False
                 mm_httpd = None
 
                 if test_config['name'] == 'media_tests':
                     from startup_test.media import media_manager
                     mm_httpd = media_manager.run_server(
                         os.path.dirname(os.path.realpath(__file__))
                     )
 
@@ -261,41 +231,39 @@ class TTest(object):
                     logfile=browser_config['browser_log'],
                     suppress_javascript_errors=True,
                     wait_for_quit_timeout=5
                 )
                 browser.run(timeout=timeout)
                 pid = browser.pid
                 self._pids.append(pid)
 
-                if self.counters:
-                    self.cm = self.CounterManager(
+                counter_management = None
+                if counters:
+                    counter_management = CounterManagement(
                         browser_config['process'],
-                        self.counters
+                        counters,
+                        resolution
                     )
-                    self.counter_results = \
-                        dict([(counter, []) for counter in self.counters])
-                    cmthread = Thread(target=self.collectCounters)
-                    cmthread.setDaemon(True)  # don't hang on quit
-                    cmthread.start()
 
                 # todo: ctrl+c doesn't close the browser windows
                 try:
                     code = browser.wait()
                 except KeyboardInterrupt:
                     browser.kill()
                     raise
+                finally:
+                    if counter_management:
+                        counter_management.stop()
+                    if mm_httpd:
+                        mm_httpd.stop()
                 logging.info(
                     "Browser exited with error code: {0}".format(code)
                 )
                 browser = None
-                self.isFinished = True
-
-                if mm_httpd:
-                    mm_httpd.stop()
 
                 if test_config['mainthread']:
                     rawlog = os.path.join(here, "mainthread_io.log")
                     if os.path.exists(rawlog):
                         processedlog = \
                             os.path.join(here, 'mainthread_io.json')
                         xre_path = \
                             os.path.dirname(browser_config['browser_path'])
@@ -349,23 +317,26 @@ class TTest(object):
                    mainthread_error_count > 0:
                     raise TalosRegression(
                         "Talos has found a regression, if you have questions"
                         " ask for help in irc on #perf"
                     )
 
                 # add the results from the browser output
                 try:
-                    test_results.add(browser_log_filename,
-                                     counter_results=self.counter_results)
-                except Exception as e:
+                    test_results.add(
+                        browser_log_filename,
+                        counter_results=(counter_management.results()
+                                         if counter_management
+                                         else None))
+                except Exception:
                     # Log the exception, but continue. One way to get here
                     # is if the browser hangs, and we'd still like to get
                     # symbolicated profiles in that case.
-                    logging.info(e)
+                    logging.exception("Unable to add results for cycle %d" % i)
 
                 if setup.sps_profile:
                     setup.sps_profile.symbolicate(i)
 
                 # clean up any stray browser processes
                 self.cleanupAndCheckForCrashes(browser_config,
                                                setup.profile_dir,
                                                test_config['name'])
@@ -377,13 +348,11 @@ class TTest(object):
             for c in test_results.all_counter_results:
                 for key, value in c.items():
                     print "COUNTER: %s" % key
                     print value
 
             # return results
             return test_results
 
-        except Exception, e:
-            self.counters = vars().get('cm', self.counters)
-            self.testCleanup(browser_config, setup.profile_dir, test_config,
-                             self.counters)
+        except Exception:
+            self.testCleanup(browser_config, setup.profile_dir, test_config)
             raise