384341 - (talos-mac) Port Talos to Mac

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Zach Lipton [:zach]]

Talos has Windows and Linux, but no Mac support yet. A cross-platform app needs a cross-platform performance testing framework! 

Current status:
- Ts numbers are being reported, but we're getting a bunch of double-restarts on startup which is inflating the numbers. Experimenting with ways to make this not happen. 
- Tp times are being reported. Need to do CPU and Memory usage. 

We'll need a machine to start running these on once I have the remaining tests implemented.

Comment 1

[Rob Campbell [:rc] (:robcee)]

we've got several mac minis parked at the colo for just this purpose. I don't think anyone's claimed them from us yet.

Comment 2

[Zach Lipton [:zach]]

Quick question as I prepare a patch for review: in the course of developing this, I added some debugging code to Talos in the form of debug() statements get printed to the console when a debug switch is turned on in the config file. I'm ok with striping these out (the current set of debug statements is fairly adhoc), but figure they may be useful to people. Do you guys have any preference?

We're still waiting on memory for the minis, but I'm going to go ahead and get a patch ready to get the review process going.

Comment 3

[Zach Lipton [:zach]]

Attachment: Patch v1.0

Here's an initial patch for Talos on Mac. This has been tested on qm-pmac01, but not on Windows or Linux. 

In addition to the changes in the attached patch, I'm also proposing (as discussed with rhelmer and some others on irc) to clean out talos/base_profile of everything except for prefs.js. Right now, there's a lot of junk in there, including Google Accelerator references, and it makes a lot more sense to let the browser generate these files instead of including them in the Talos distribution. I'm not sure if we can do this without breaking the baselining however.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Note: bug 385404 has added browser height and width configuration parameters that get used in ffprocess_linux|win32.py::GenerateFirefoxCommandLine.  I think you'll need to add those to ffprocess_mac.py as well.

Also, I'm adding OS-autodetection in bug 387489, which just means that instead of detecting the Mac OS like this:

elif config.OS == "mac":
  from ffprocess_mac import *

You'll detect it like this:

elif platform.system() == "Darwin":
  from ffprocess_mac import *

Comment 5

[Zach Lipton [:zach]]

Comment on attachment 271124 [details] [diff] [review]
Patch v1.0

Thanks Myk. I'll have an updated patch tomorrow for your review Alice.

Comment 6

[Zach Lipton [:zach]]

Going to wait on bug 387489 (Myk's platform auto-detection) since that patch will break mine.

Comment 7

[Zach Lipton [:zach]]

Attachment: Patch v1.1

Here's an updated patch that incorporates Myk's platform-detection change.

Comment 8

[Zach Lipton [:zach]]

Attachment: Patch v2: Windows/Linux fixes

As promised, here's a new patch that behaves properly on windows and linux.

Comment 9

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

Comment on attachment 272260 [details] [diff] [review]
Patch v2: Windows/Linux fixes

Looks like you've got two separate copies of quit.js in there.  Also, the license block in quit.js needs to be fixed up.

In tp_mac.py, I'm concerned about:

+def GetProcessData(pid):
+  """Runs a ps on the process identified by pid and returns the output line
+    as a list (uid, pid, ppid, cpu, pri, ni, vsz, rss, wchan, stat, tt, time, command)
+  """
+  command = ['ps -Acup'+str(pid)]
+  handle = subprocess.Popen(command, stdout=subprocess.PIPE, universal_newlines=True, shell=True)
+  handle.wait()
+  data = handle.stdout.readlines()
+  
+  # find all matching processes and add them to the list
+  for line in data:
+    if line.find(str(pid)) >= 0:
+      # splits by whitespace
+      line = line.split()
+      if (line[1] == str(pid)):
+        return line

Constantly requesting/waiting/reading from ps could cause performance woes of its own.  I talked with robcee about this briefly and we aren't sure if there is a better way to go about this - I'd be interested in seeing it do a run on a mac with the Activity Monitor open to see what the effects of this are.

Comment 10

[Zach Lipton [:zach]]

Attachment: Patch v3

Here's a new patch with the following changes: 

- Only one copy of quit.js, living in the page_load_test/ directory as per our discussion
- Tautology removed from quit.js license block
- CPU usage numbers just return 0 now for mac and linux as per today's perf meeting (all that work yesterday to redo cpu usage down the drain ;)

Comment 11

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

Comment on attachment 272818 [details] [diff] [review]
Patch v3

in tp_linux.py
-  try:
-    activeCpuTime = float(states.count("R")) / float(len(states))
-  except ZeroDivisionError:
-    activeCpuTime = -1
-
-  
-  return float("%.2lf" % (activeCpuTime * 100))
+  # return all zeros on this platform as per the 7/18/07 perf meeting
 

This doesn't look like it is returning anything at all.

in ffprofile_mac
+def MakeDirectoryContentsWritable(dirname):
+  """Recursively makes all the contents of a directory writable.
+     Uses os.chmod(filename, 0755).
+      
+  Args:
+    dirname: Name of the directory to make contents writable.
+  """
+
+  for (root, dirs, files) in os.walk(dirname):
+    os.chmod(root, 0755)
+    for filename in files:
+      os.chmod(os.path.join(root, filename), 0755)

The exact same code is used for ffprofile_linux.py - is it really necessary to have this in a separate file again?  Maybe just have ffprofile_win32 and ffprofile_other.

What does setting/re-setting the environment vars do?  I'm not sure if I see any being set - are these specific ones necessary for mac runs?

I think that we are getting down to the end though - it is looking pretty solid.

Comment 12

[Zach Lipton [:zach]]

Attachment: Patch v3.5

Here's a new patch. Changes:

- Fixed the missing return statement in the linux cpu numbers
- Moved all the profile code into ffprofile_unix, shared between linux and macos. 
- Added environment variable settings to sample.config

The localstore.rdf issue mentioned earlier on irc seems to be ok, I believe just a temporary issue with that one build, so I'm not inclined to change anything now, but I'll keep an eye on it.

Comment 13

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

Comment on attachment 272996 [details] [diff] [review]
Patch v3.5

This looks like a workable first step to having support on all three platforms - in future I think that we could get rid of some of the code overlap in various places.

Comment 14

[Zach Lipton [:zach]]

Fix was checked in Monday and all seems well.