571319 - remove threading from talos

Status: RESOLVED FIXED

(Testing :: Talos, defect, P2)

Description

[alice nodelman [:alice] [:anode]]

I do not believe that we are gaining anything by the threading currently used in talos, and we but up against non-threadsafe subprocess behavior that hampers our ability to add new features/fix bugs.

For background see here http://bugs.python.org/issue1731717

Comment 1

[alice nodelman [:alice] [:anode]]

Attachment: remove threading from talos

Initial patch for removing threading from talos.  Still needs further staging to ensure that number collection isn't harmed.

Comment 2

[Jeff Hammel]

Comment on attachment 451714 [details] [diff] [review]
remove threading from talos

>Index: cmanager_linux.py
>===================================================================
>RCS file: /cvsroot/mozilla/testing/performance/talos/cmanager_linux.py,v
>retrieving revision 1.10
>diff -u -8 -p -r1.10 cmanager_linux.py
>--- cmanager_linux.py	15 Apr 2010 18:00:15 -0000	1.10
>+++ cmanager_linux.py	16 Jun 2010 00:02:42 -0000
>@@ -17,16 +17,17 @@
> #
> # The Initial Developer of the Original Code is Google Inc.
> # Portions created by the Initial Developer are Copyright (C) 2006
> # the Initial Developer. All Rights Reserved.
> #
> # Contributor(s):
> #   Annie Sullivan <[e-mail]> (original author)
> #   Ben Hearsum    <[e-mail]> (ported to linux)
>+#   Alice Nodelman    <[e-mail]> (removed threading)
> #
> # Alternatively, the contents of this file may be used under the terms of
> # either the GNU General Public License Version 2 or later (the "GPL"), or
> # the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
> # in which case the provisions of the GPL or the LGPL are applicable instead
> # of those above. If you wish to allow use of your version of this file only
> # under the terms of either the GPL or the LGPL, and not to allow others to
> # use your version of this file under the terms of the MPL, indicate your
>@@ -38,17 +39,16 @@
> # ***** END LICENSE BLOCK *****
> 
> __author__ = '[e-mail] (Annie Sullivan)'
> 
> import subprocess
> import sys
> import os
> import time
>-import threading
> 
> def GetPrivateBytes(pids):
>   """Calculate the amount of private, writeable memory allocated to a process.
>      This code was adapted from 'pmap.c', part of the procps project.
>   """
>   privateBytes = 0
>   for pid in pids:
>     mapfile = '/proc/%s/maps' % pid
>@@ -120,17 +120,17 @@ def GetXRes(pids):
>   return XRes
> 
> counterDict = {}
> counterDict["Private Bytes"] = GetPrivateBytes
> counterDict["RSS"] = GetResidentSize
> counterDict["% Processor Time"] = GetCpuTime
> counterDict["XRes"] = GetXRes
> 
>-class CounterManager(threading.Thread):
>+class CounterManager():
>   """This class manages the monitoring of a process with any number of
>      counters.
> 
>      A counter can be any function that takes an argument of one pid and
>      returns a piece of data about that process.
>      Some examples are: CalcCPUTime, GetResidentSize, and GetPrivateBytes
>   """
>   
>@@ -138,28 +138,26 @@ class CounterManager(threading.Thread):
> 
>   def __init__(self, ffprocess, process, counters=None, childProcess="plugin-container"):
>     """Args:
>          counters: A list of counters to monitor. Any counters whose name does
>          not match a key in 'counterDict' will be ignored.
>     """
>     self.allCounters = {}
>     self.registeredCounters = {}
>-    self.process = process
>     self.childProcess = childProcess
>     self.runThread = False
>-    self.primaryPid = -1
>     self.pidList = []
>     self.ffprocess = ffprocess
>+    self.primaryPid = self.ffprocess.GetPidsByName(process)[-1]
>+    os.stat('/proc/%s' % self.primaryPid)
> 
>     self._loadCounters()
>     self.registerCounters(counters)
> 
>-    threading.Thread.__init__(self)
>-
Unless i'm mistaken, process is passed but not used.  If it's not used, it probably shouldn' be passed.

>   def _loadCounters(self):
>     """Loads all of the counters defined in the counterDict"""
>     for counter in counterDict.keys():
>       self.allCounters[counter] = counterDict[counter]
> 
>   def registerCounters(self, counters):
>     """Registers a list of counters that will be monitoring.
>        Only counters whose names are found in allCounters will be added
>@@ -180,79 +178,29 @@ class CounterManager(threading.Thread):
> 
>   def getRegisteredCounters(self):
>     """Returns a list of the registered counters."""
>     return keys(self.registeredCounters)
> 
>   def getCounterValue(self, counterName):
>     """Returns the last value of the counter 'counterName'"""
>     try:
>-      if counterName is "% Processor Time":
>-        return self._getCounterAverage(counterName)
>-      else:
>-        return self.registeredCounters[counterName][1][-1]
>-    except:
>-      return None
>-
>-  def _getCounterAverage(self, counterName):
>-    """Returns the average value of the counter 'counterName'"""
>-    try:
>-      total = 0
>-      for v in self.registeredCounters[counterName][1]:
>-        total += v
>-      return total / len(self.registeredCounters[counterName][1])
>+      self.updatePidList()
>+      return self.registeredCounters[counterName][0](self.pidList)
>     except:
>       return None
> 
>-  def getProcess(self):
>-    """Returns the process currently associated with this CounterManager"""
>-    return self.process
>-
>   def updatePidList(self):
>     """Updates the list of PIDs we're interested in"""
>     try:
>       self.pidList = [self.primaryPid]
>       childPids = self.ffprocess.GetPidsByName(self.childProcess)
>       for pid in childPids:
>         os.stat('/proc/%s' % pid)
>         self.pidList.append(pid)
>     except:
>       print "WARNING: problem updating child PID's"
> 
>-  def startMonitor(self):
>-    """Starts the monitoring process.
>-       Throws an exception if any error occurs
>-    """
>-    # TODO: make this function less ugly
>-    try:
>-      # the last process is the useful one
>-      self.primaryPid = self.ffprocess.GetPidsByName(self.process)[-1]
>-      os.stat('/proc/%s' % self.primaryPid)
>-      self.runThread = True
>-      self.start()
>-    except:
>-      print 'WARNING: problem starting counter monitor'
>-
>   def stopMonitor(self):
>-    """Stops the monitor"""
>+    """any final cleanup"""
>     # TODO: should probably wait until we know run() is completely stopped
>     # before setting self.pid to None. Use a lock?
>-    self.runThread = False
>-
>-  def run(self):
>-    """Performs the actual monitoring of the process. Will keep running
>-       until stopMonitor() is called
>-    """
>-    while self.runThread:
>-      self.updatePidList()
>-      for counter in self.registeredCounters.keys():
>-        # counter[0] is a function that gets the current value for
>-        # a counter
>-        # counter[1] is a list of recorded values
>-        try:
>-          self.registeredCounters[counter][1].append(
>-            self.registeredCounters[counter][0](self.pidList))
>-        except:
>-          # if a counter throws an exception, remove it
>-          #self.unregisterCounters([counter])
>-          print "Error in collecting counter: " + counter
>-
>-      time.sleep(self.pollInterval)
>+    return
>Index: cmanager_mac.py
>===================================================================
>RCS file: /cvsroot/mozilla/testing/performance/talos/cmanager_mac.py,v
>retrieving revision 1.7
>diff -u -8 -p -r1.7 cmanager_mac.py
>--- cmanager_mac.py	14 Jan 2010 19:04:23 -0000	1.7
>+++ cmanager_mac.py	16 Jun 2010 00:02:42 -0000
>@@ -18,16 +18,17 @@
> # The Initial Developer of the Original Code is Google Inc.
> # Portions created by the Initial Developer are Copyright (C) 2006
> # the Initial Developer. All Rights Reserved.
> #
> # Contributor(s):
> #   Annie Sullivan <[e-mail]> (original author)
> #   Ben Hearsum    <[e-mail]> (ported to linux)
> #   Zach Lipton    <[e-mail]>  (Mac port)
>+#   Alice Nodelman    <[e-mail]>  (removed threading)
> #
> # Alternatively, the contents of this file may be used under the terms of
> # either the GNU General Public License Version 2 or later (the "GPL"), or
> # the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
> # in which case the provisions of the GPL or the LGPL are applicable instead
> # of those above. If you wish to allow use of your version of this file only
> # under the terms of either the GPL or the LGPL, and not to allow others to
> # use your version of this file under the terms of the MPL, indicate your
>@@ -38,17 +39,16 @@
> #
> # ***** END LICENSE BLOCK *****
> 
> __author__ = '[e-mail] (Annie Sullivan)'
> 
> 
> import os
> import time
>-import threading
> import subprocess
> 
> def GetProcessData(pid):
>   """Runs a ps on the process identified by pid and returns the output line
>     as a list (pid, vsz, rss)
>   """
>   command = ['ps -o pid,vsize,rss -p'+str(pid)]
>   handle = subprocess.Popen(command, stdout=subprocess.PIPE, universal_newlines=True, shell=True)
>@@ -79,44 +79,39 @@ def GetCpuTime(pid):
>   # return all zeros for now on this platform as per 7/18/07 perf meeting
>   return 0
> 
> counterDict = {}
> counterDict["Private Bytes"] = GetPrivateBytes
> counterDict["RSS"] = GetResidentSize
> counterDict["% Processor Time"] = GetCpuTime
> 
>-class CounterManager(threading.Thread):
>+class CounterManager():
>   """This class manages the monitoring of a process with any number of
>      counters.
> 
>      A counter can be any function that takes an argument of one pid and
>      returns a piece of data about that process.
>      Some examples are: CalcCPUTime, GetResidentSize, and GetPrivateBytes
>   """
>   
>-  pollInterval = .25
>-
>   def __init__(self, ffprocess, process, counters=None):
>     """Args:
>          counters: A list of counters to monitor. Any counters whose name does
>          not match a key in 'counterDict' will be ignored.
>     """
>     self.allCounters = {}
>     self.registeredCounters = {}
>-    self.process = process
>-    self.runThread = False
>-    self.pid = -1
>     self.ffprocess = ffprocess
>+    # the last process is the useful one
>+    self.pid = self.ffprocess.GetPidsByName(process)[-1]
> 
>     self._loadCounters()
>     self.registerCounters(counters)
> 
>-    threading.Thread.__init__(self)
>-

Same as above re passing process but not using it.

>   def _loadCounters(self):
>     """Loads all of the counters defined in the counterDict"""
>     for counter in counterDict.keys():
>       self.allCounters[counter] = counterDict[counter]
> 
>   def registerCounters(self, counters):
>     """Registers a list of counters that will be monitoring.
>        Only counters whose names are found in allCounters will be added
>@@ -137,66 +132,16 @@ class CounterManager(threading.Thread):
> 
>   def getRegisteredCounters(self):
>     """Returns a list of the registered counters."""
>     return keys(self.registeredCounters)
> 
>   def getCounterValue(self, counterName):
>     """Returns the last value of the counter 'counterName'"""
>     try:
>-      if counterName is "% Processor Time":
>-        return self._getCounterAverage(counterName)
>-      else:
>-        return self.registeredCounters[counterName][1][-1]
>-    except:
>-      return None
>-
>-  def _getCounterAverage(self, counterName):
>-    """Returns the average value of the counter 'counterName'"""
>-    try:
>-      total = 0
>-      for v in self.registeredCounters[counterName][1]:
>-        total += v
>-      return total / len(self.registeredCounters[counterName][1])
>+      return self.registeredCounters[counterName][0](self.pid)
>     except:
>+      print "Error in collecting counter: " + counterName
>       return None

Is there a reason why these excepts are so permissive?

>-  def getProcess(self):
>-    """Returns the process currently associated with this CounterManager"""
>-    return self.process
>-
>-  def startMonitor(self):
>-    """Starts the monitoring process.
>-       Throws an exception if any error occurs
>-    """
>-    # TODO: make this function less ugly
>-    try:
>-      # the last process is the useful one
>-      self.pid = self.ffprocess.GetPidsByName(self.process)[-1]
>-      self.runThread = True
>-      self.start()
>-    except:
>-      print 'WARNING: problem starting counter monitor'
>-
>   def stopMonitor(self):
>-    """Stops the monitor"""
>-    # TODO: should probably wait until we know run() is completely stopped
>-    # before setting self.pid to None. Use a lock?
>-    self.runThread = False
>-
>-  def run(self):
>-    """Performs the actual monitoring of the process. Will keep running
>-       until stopMonitor() is called
>-    """
>-    while self.runThread:
>-      for counter in self.registeredCounters.keys():
>-        # counter[0] is a function that gets the current value for
>-        # a counter
>-        # counter[1] is a list of recorded values
>-        try:
>-          self.registeredCounters[counter][1].append(
>-            self.registeredCounters[counter][0](self.pid))
>-        except:
>-          # if a counter throws an exception, remove it
>-          #self.unregisterCounters([counter]) #don't remove, let it try and resolve on next cycle
>-          print "Error in collecting counter: " + counter
>-
>-      time.sleep(self.pollInterval)
>+     """any final cleanup"""
>+     return
>Index: ttest.py
>===================================================================
>RCS file: /cvsroot/mozilla/testing/performance/talos/ttest.py,v
>retrieving revision 1.44
>diff -u -8 -p -r1.44 ttest.py
>--- ttest.py	7 Jun 2010 21:55:24 -0000	1.44
>+++ ttest.py	16 Jun 2010 00:02:42 -0000
>@@ -272,17 +272,16 @@ class TTest(object):
>                 #give browser a chance to open
>                 # this could mean that we are losing the first couple of data points
>                 #   as the tests starts, but if we don't provide
>                 # some time for the browser to start we have trouble connecting the CounterManager to it
>                 time.sleep(browser_config['browser_wait'])
>                 #set up the counters for this test
>                 if counters:
>                     cm = self.cmanager.CounterManager(self._ffprocess, browser_config['process'], counters)
>-                    cm.startMonitor()
>                 counter_results = {}
>                 for counter in counters:
>                     counter_results[counter] = []
> 
>                 startTime = -1
>                 dumpResult = ""
>                 #the main test loop, monitors counters and checks for browser ouptut
>                 while total_time < timeout:

Far from a thorough review, but looks fine.

Comment 3

[alice nodelman [:alice] [:anode]]

Attachment: [checked in]remove threading from talos (take 2)

bhearsum - tagging you for review since you wrote some of this code.  ha!

Comment 4

[Jeff Hammel]

Comment on attachment 453872 [details] [diff] [review]
[checked in]remove threading from talos (take 2)

>Index: cmanager_linux.py
>===================================================================
>RCS file: /cvsroot/mozilla/testing/performance/talos/cmanager_linux.py,v
>retrieving revision 1.10
>diff -u -8 -p -r1.10 cmanager_linux.py
>--- cmanager_linux.py	15 Apr 2010 18:00:15 -0000	1.10
>+++ cmanager_linux.py	23 Jun 2010 22:52:19 -0000
>@@ -17,16 +17,17 @@
> #
> # The Initial Developer of the Original Code is Google Inc.
> # Portions created by the Initial Developer are Copyright (C) 2006
> # the Initial Developer. All Rights Reserved.
> #
> # Contributor(s):
> #   Annie Sullivan <[e-mail]> (original author)
> #   Ben Hearsum    <[e-mail]> (ported to linux)
>+#   Alice Nodelman    <[e-mail]> (removed threading)
> #
> # Alternatively, the contents of this file may be used under the terms of
> # either the GNU General Public License Version 2 or later (the "GPL"), or
> # the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
> # in which case the provisions of the GPL or the LGPL are applicable instead
> # of those above. If you wish to allow use of your version of this file only
> # under the terms of either the GPL or the LGPL, and not to allow others to
> # use your version of this file under the terms of the MPL, indicate your
>@@ -38,17 +39,16 @@
> # ***** END LICENSE BLOCK *****
> 
> __author__ = '[e-mail] (Annie Sullivan)'
> 
> import subprocess
> import sys
> import os
> import time
>-import threading
> 
> def GetPrivateBytes(pids):
>   """Calculate the amount of private, writeable memory allocated to a process.
>      This code was adapted from 'pmap.c', part of the procps project.
>   """
>   privateBytes = 0
>   for pid in pids:
>     mapfile = '/proc/%s/maps' % pid
>@@ -120,17 +120,17 @@ def GetXRes(pids):
>   return XRes
> 
> counterDict = {}
> counterDict["Private Bytes"] = GetPrivateBytes
> counterDict["RSS"] = GetResidentSize
> counterDict["% Processor Time"] = GetCpuTime
> counterDict["XRes"] = GetXRes
> 
>-class CounterManager(threading.Thread):
>+class CounterManager():
>   """This class manages the monitoring of a process with any number of
>      counters.
> 
>      A counter can be any function that takes an argument of one pid and
>      returns a piece of data about that process.
>      Some examples are: CalcCPUTime, GetResidentSize, and GetPrivateBytes
>   """
>   
>@@ -138,28 +138,26 @@ class CounterManager(threading.Thread):
> 
>   def __init__(self, ffprocess, process, counters=None, childProcess="plugin-container"):
>     """Args:
>          counters: A list of counters to monitor. Any counters whose name does
>          not match a key in 'counterDict' will be ignored.
>     """
>     self.allCounters = {}
>     self.registeredCounters = {}
>-    self.process = process
>     self.childProcess = childProcess
>     self.runThread = False
>-    self.primaryPid = -1
>     self.pidList = []
>     self.ffprocess = ffprocess
>+    self.primaryPid = self.ffprocess.GetPidsByName(process)[-1]
>+    os.stat('/proc/%s' % self.primaryPid)
 what is the purpose of this line?  you call stat but don't keep the stat result.

> 
>     self._loadCounters()
>     self.registerCounters(counters)
> 
>-    threading.Thread.__init__(self)
>-
>   def _loadCounters(self):
>     """Loads all of the counters defined in the counterDict"""
>     for counter in counterDict.keys():
>       self.allCounters[counter] = counterDict[counter]
> 
>   def registerCounters(self, counters):
>     """Registers a list of counters that will be monitoring.
>        Only counters whose names are found in allCounters will be added
>@@ -180,79 +178,29 @@ class CounterManager(threading.Thread):
> 
>   def getRegisteredCounters(self):
>     """Returns a list of the registered counters."""
>     return keys(self.registeredCounters)
> 
>   def getCounterValue(self, counterName):
>     """Returns the last value of the counter 'counterName'"""
>     try:
>-      if counterName is "% Processor Time":
>-        return self._getCounterAverage(counterName)
>-      else:
>-        return self.registeredCounters[counterName][1][-1]
>-    except:
>-      return None
>-
>-  def _getCounterAverage(self, counterName):
>-    """Returns the average value of the counter 'counterName'"""
>-    try:
>-      total = 0
>-      for v in self.registeredCounters[counterName][1]:
>-        total += v
>-      return total / len(self.registeredCounters[counterName][1])
>+      self.updatePidList()
>+      return self.registeredCounters[counterName][0](self.pidList)
>     except:
>       return None
> 
>-  def getProcess(self):
>-    """Returns the process currently associated with this CounterManager"""
>-    return self.process
>-
>   def updatePidList(self):
>     """Updates the list of PIDs we're interested in"""
>     try:
>       self.pidList = [self.primaryPid]
>       childPids = self.ffprocess.GetPidsByName(self.childProcess)
>       for pid in childPids:
>         os.stat('/proc/%s' % pid)
>         self.pidList.append(pid)
>     except:
>       print "WARNING: problem updating child PID's"
> 
>-  def startMonitor(self):
>-    """Starts the monitoring process.
>-       Throws an exception if any error occurs
>-    """
>-    # TODO: make this function less ugly
>-    try:
>-      # the last process is the useful one
>-      self.primaryPid = self.ffprocess.GetPidsByName(self.process)[-1]
>-      os.stat('/proc/%s' % self.primaryPid)
>-      self.runThread = True
>-      self.start()
>-    except:
>-      print 'WARNING: problem starting counter monitor'
>-
>   def stopMonitor(self):
>-    """Stops the monitor"""
>+    """any final cleanup"""

Should probably change the function name if the intend is different.  Does this function even need to be here now?

>     # TODO: should probably wait until we know run() is completely stopped
>     # before setting self.pid to None. Use a lock?
>-    self.runThread = False
>-
>-  def run(self):
>-    """Performs the actual monitoring of the process. Will keep running
>-       until stopMonitor() is called
>-    """
>-    while self.runThread:
>-      self.updatePidList()
>-      for counter in self.registeredCounters.keys():
>-        # counter[0] is a function that gets the current value for
>-        # a counter
>-        # counter[1] is a list of recorded values
>-        try:
>-          self.registeredCounters[counter][1].append(
>-            self.registeredCounters[counter][0](self.pidList))
>-        except:
>-          # if a counter throws an exception, remove it
>-          #self.unregisterCounters([counter])
>-          print "Error in collecting counter: " + counter
>-
>-      time.sleep(self.pollInterval)
>+    return
No need to call return if you're just returning None.

>Index: cmanager_mac.py
>===================================================================
>RCS file: /cvsroot/mozilla/testing/performance/talos/cmanager_mac.py,v
>retrieving revision 1.7
>diff -u -8 -p -r1.7 cmanager_mac.py
>--- cmanager_mac.py	14 Jan 2010 19:04:23 -0000	1.7
>+++ cmanager_mac.py	23 Jun 2010 22:52:19 -0000
>@@ -18,16 +18,17 @@
> # The Initial Developer of the Original Code is Google Inc.
> # Portions created by the Initial Developer are Copyright (C) 2006
> # the Initial Developer. All Rights Reserved.
> #
> # Contributor(s):
> #   Annie Sullivan <[e-mail]> (original author)
> #   Ben Hearsum    <[e-mail]> (ported to linux)
> #   Zach Lipton    <[e-mail]>  (Mac port)
>+#   Alice Nodelman    <[e-mail]>  (removed threading)
> #
> # Alternatively, the contents of this file may be used under the terms of
> # either the GNU General Public License Version 2 or later (the "GPL"), or
> # the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
> # in which case the provisions of the GPL or the LGPL are applicable instead
> # of those above. If you wish to allow use of your version of this file only
> # under the terms of either the GPL or the LGPL, and not to allow others to
> # use your version of this file under the terms of the MPL, indicate your
>@@ -38,17 +39,16 @@
> #
> # ***** END LICENSE BLOCK *****
> 
> __author__ = '[e-mail] (Annie Sullivan)'
> 
> 
> import os
> import time
>-import threading
> import subprocess
> 
> def GetProcessData(pid):
>   """Runs a ps on the process identified by pid and returns the output line
>     as a list (pid, vsz, rss)
>   """
>   command = ['ps -o pid,vsize,rss -p'+str(pid)]
>   handle = subprocess.Popen(command, stdout=subprocess.PIPE, universal_newlines=True, shell=True)
>@@ -79,44 +79,39 @@ def GetCpuTime(pid):
>   # return all zeros for now on this platform as per 7/18/07 perf meeting
>   return 0
> 
> counterDict = {}
> counterDict["Private Bytes"] = GetPrivateBytes
> counterDict["RSS"] = GetResidentSize
> counterDict["% Processor Time"] = GetCpuTime
> 
>-class CounterManager(threading.Thread):
>+class CounterManager():
>   """This class manages the monitoring of a process with any number of
>      counters.
> 
>      A counter can be any function that takes an argument of one pid and
>      returns a piece of data about that process.
>      Some examples are: CalcCPUTime, GetResidentSize, and GetPrivateBytes
>   """
>   
>-  pollInterval = .25
>-
>   def __init__(self, ffprocess, process, counters=None):
>     """Args:
>          counters: A list of counters to monitor. Any counters whose name does
>          not match a key in 'counterDict' will be ignored.
>     """
>     self.allCounters = {}
>     self.registeredCounters = {}
>-    self.process = process
>-    self.runThread = False
>-    self.pid = -1
>     self.ffprocess = ffprocess
>+    # the last process is the useful one
>+    self.pid = self.ffprocess.GetPidsByName(process)[-1]
> 
>     self._loadCounters()
>     self.registerCounters(counters)
> 
>-    threading.Thread.__init__(self)
>-
>   def _loadCounters(self):
>     """Loads all of the counters defined in the counterDict"""
>     for counter in counterDict.keys():
>       self.allCounters[counter] = counterDict[counter]
> 
>   def registerCounters(self, counters):
>     """Registers a list of counters that will be monitoring.
>        Only counters whose names are found in allCounters will be added
>@@ -137,66 +132,16 @@ class CounterManager(threading.Thread):
> 
>   def getRegisteredCounters(self):
>     """Returns a list of the registered counters."""
>     return keys(self.registeredCounters)
> 
>   def getCounterValue(self, counterName):
>     """Returns the last value of the counter 'counterName'"""
>     try:
>-      if counterName is "% Processor Time":
>-        return self._getCounterAverage(counterName)
>-      else:
>-        return self.registeredCounters[counterName][1][-1]
>-    except:
>-      return None
>-
>-  def _getCounterAverage(self, counterName):
>-    """Returns the average value of the counter 'counterName'"""
>-    try:
>-      total = 0
>-      for v in self.registeredCounters[counterName][1]:
>-        total += v
>-      return total / len(self.registeredCounters[counterName][1])
>+      return self.registeredCounters[counterName][0](self.pid)
>     except:
>+      print "Error in collecting counter: " + counterName
>       return None
> 
>-  def getProcess(self):
>-    """Returns the process currently associated with this CounterManager"""
>-    return self.process
>-
>-  def startMonitor(self):
>-    """Starts the monitoring process.
>-       Throws an exception if any error occurs
>-    """
>-    # TODO: make this function less ugly
>-    try:
>-      # the last process is the useful one
>-      self.pid = self.ffprocess.GetPidsByName(self.process)[-1]
>-      self.runThread = True
>-      self.start()
>-    except:
>-      print 'WARNING: problem starting counter monitor'
>-
>   def stopMonitor(self):
>-    """Stops the monitor"""
>-    # TODO: should probably wait until we know run() is completely stopped
>-    # before setting self.pid to None. Use a lock?
>-    self.runThread = False
>-
>-  def run(self):
>-    """Performs the actual monitoring of the process. Will keep running
>-       until stopMonitor() is called
>-    """
>-    while self.runThread:
>-      for counter in self.registeredCounters.keys():
>-        # counter[0] is a function that gets the current value for
>-        # a counter
>-        # counter[1] is a list of recorded values
>-        try:
>-          self.registeredCounters[counter][1].append(
>-            self.registeredCounters[counter][0](self.pid))
>-        except:
>-          # if a counter throws an exception, remove it
>-          #self.unregisterCounters([counter]) #don't remove, let it try and resolve on next cycle
>-          print "Error in collecting counter: " + counter
>-
>-      time.sleep(self.pollInterval)
>+     """any final cleanup"""
>+     return
>Index: cmanager_win32.py
>===================================================================
>RCS file: /cvsroot/mozilla/testing/performance/talos/cmanager_win32.py,v
>retrieving revision 1.11
>diff -u -8 -p -r1.11 cmanager_win32.py
>--- cmanager_win32.py	7 Jun 2010 21:55:24 -0000	1.11
>+++ cmanager_win32.py	23 Jun 2010 22:52:19 -0000
>@@ -38,25 +38,45 @@
> 
> import win32pdh
> import win32pdhutil
> 
> 
> class CounterManager:
> 
>   def __init__(self, ffprocess, process, counters=None, childProcess="plugin-container"):
>-    self.process = process
>     self.ffprocess = ffprocess
>     self.childProcess = childProcess
>     self.registeredCounters = {}
>     self.registerCounters(counters)
>     # PDH might need to be "refreshed" if it has been queried while the browser
>     # is closed
>     win32pdh.EnumObjects(None, None, 0, 1)
> 
>+    # Add the counter path for the default process.
>+    for counter in self.registeredCounters:
>+      path = win32pdh.MakeCounterPath((None, 'process', process,
>+                                       None, -1, counter))
>+      hq = win32pdh.OpenQuery()
>+      try:
>+        hc = win32pdh.AddCounter(hq, path)
>+      except:
>+        win32pdh.CloseQuery(hq)
>+        #assume that this is a memory counter for the system, not a process counter
>+        path = win32pdh.MakeCounterPath((None, 'Memory', None, None, -1 , counter))
>+        hq = win32pdh.OpenQuery()  
>+        try:                                                                       
>+          hc = win32pdh.AddCounter(hq, path)                                       
>+        except:                                                                    
>+          win32pdh.CloseQuery(hq)    
>+
>+      self.registeredCounters[counter] = [hq, [(hc, path)]]
>+      self.updateCounterPathsForChildProcesses(counter)
>+
>+
>   def registerCounters(self, counters):
>     # self.registeredCounters[counter][0] is a counter query handle
>     # self.registeredCounters[counter][1] is a list of tuples, the first 
>     # member of which is a counter handle, the second a counter path
>     for counter in counters:
>       self.registeredCounters[counter] = []
>             
>   def unregisterCounters(self, counters):
>@@ -122,41 +142,16 @@ class CounterManager:
>         val = 0
>       aggregateValue += val
> 
>     return aggregateValue
> 
>   def getProcess(self):
>     return self.process
> 
>-  def startMonitor(self):
>-    # PDH might need to be "refreshed" if it has been queried while the browser
>-    # is closed
>-    win32pdh.EnumObjects(None, None, 0, 1)
>-
>-    # Add the counter path for the default process.
>-    for counter in self.registeredCounters:
>-      path = win32pdh.MakeCounterPath((None, 'process', self.process,
>-                                       None, -1, counter))
>-      hq = win32pdh.OpenQuery()
>-      try:
>-        hc = win32pdh.AddCounter(hq, path)
>-      except:
>-        win32pdh.CloseQuery(hq)
>-        #assume that this is a memory counter for the system, not a process counter
>-        path = win32pdh.MakeCounterPath((None, 'Memory', None, None, -1 , counter))
>-        hq = win32pdh.OpenQuery()  
>-        try:                                                                       
>-          hc = win32pdh.AddCounter(hq, path)                                       
>-        except:                                                                    
>-          win32pdh.CloseQuery(hq)    
>-
>-      self.registeredCounters[counter] = [hq, [(hc, path)]]
>-      self.updateCounterPathsForChildProcesses(counter)
>-
>   def stopMonitor(self):
>     try:
>       for counter in self.registeredCounters:
>         for singleCounter in self.registeredCounters[counter][1]: 
>           win32pdh.RemoveCounter(singleCounter[0])
>         win32pdh.CloseQuery(self.registeredCounters[counter][0])
>       self.registeredCounters.clear()
>     except:
>Index: ttest.py
>===================================================================
>RCS file: /cvsroot/mozilla/testing/performance/talos/ttest.py,v
>retrieving revision 1.44
>diff -u -8 -p -r1.44 ttest.py
>--- ttest.py	7 Jun 2010 21:55:24 -0000	1.44
>+++ ttest.py	23 Jun 2010 22:52:19 -0000
>@@ -272,17 +272,16 @@ class TTest(object):
>                 #give browser a chance to open
>                 # this could mean that we are losing the first couple of data points
>                 #   as the tests starts, but if we don't provide
>                 # some time for the browser to start we have trouble connecting the CounterManager to it
>                 time.sleep(browser_config['browser_wait'])
>                 #set up the counters for this test
>                 if counters:
>                     cm = self.cmanager.CounterManager(self._ffprocess, browser_config['process'], counters)
>-                    cm.startMonitor()
>                 counter_results = {}
>                 for counter in counters:
>                     counter_results[counter] = []
> 
>                 startTime = -1
>                 dumpResult = ""
>                 #the main test loop, monitors counters and checks for browser ouptut
>                 while total_time < timeout:

Comment 5

[[e-mail] (:bhearsum)]

Comment on attachment 453872 [details] [diff] [review]
[checked in]remove threading from talos (take 2)

Looks like os.stat is used to check for existence here. Why I used that instead of os.path.exists? No good reason, I suspect!

This change looks OK to me, and the minimal change to ttest.py makes me question why threading was used in the first place.

Comment 6

[alice nodelman [:alice] [:anode]]

Comment on attachment 453872 [details] [diff] [review]
[checked in]remove threading from talos (take 2)

Checking in cmanager_mac.py;
/cvsroot/mozilla/testing/performance/talos/cmanager_mac.py,v  <--  cmanager_mac.py
new revision: 1.8; previous revision: 1.7
done
Checking in cmanager_win32.py;
/cvsroot/mozilla/testing/performance/talos/cmanager_win32.py,v  <--  cmanager_win32.py
new revision: 1.12; previous revision: 1.11
done
Checking in cmanager_linux.py;
/cvsroot/mozilla/testing/performance/talos/cmanager_linux.py,v  <--  cmanager_linux.py
new revision: 1.11; previous revision: 1.10
done
Checking in ttest.py;
/cvsroot/mozilla/testing/performance/talos/ttest.py,v  <--  ttest.py
new revision: 1.45; previous revision: 1.44
done

Comment 7

[alice nodelman [:alice] [:anode]]

Landed, all done here.

Comment 8

[Chris AtLee [:catlee]]

Required a bustage fix:
Checking in cmanager_linux.py;
/cvsroot/mozilla/testing/performance/talos/cmanager_linux.py,v  <--  cmanager_linux.py
new revision: 1.12; previous revision: 1.11
done
Checking in cmanager_mac.py;
/cvsroot/mozilla/testing/performance/talos/cmanager_mac.py,v  <--  cmanager_mac.py
new revision: 1.9; previous revision: 1.8
done