870657 - Use the profiler for Ts Talos

Status: RESOLVED DUPLICATE of bug 967635

(Core :: Gecko Profiler, defect)

Description

[Benoit Girard (:BenWa)]

Attachment: Talos

As discussed with jmaher profiling Ts first is the easiest because:
1. We have no other way of getting data out of test slaves then stdout
2. stdout is limited to 50MB or 100MB

Here's my first attempt. I tried it locally and it works great. Right now this should only work on desktop so I intentionally disallow remote configurations, let me know if this is done correctly. 

This patch will return a zlib+Base64 encoded version of the profile. It goes from 1.5MB -> 100Kb. Currently you have to Decode64+unzlib using a python shell and copy the results in http://people.mozilla.com/~bgirard/cleopatra/.

Comment 1

[Benoit Girard (:BenWa)]

I had a chat with catlee. He says that allowing an outgoing rule to our AppEngine storage for profile is a possibility. Let's go ahead with this design for now and if this approach is useful we can extend this to store profiles to AppEngine. This would allow us to use this for longer runs like tp5.

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 747758 [details] [diff] [review]
Talos

Review of attachment 747758 [details] [diff] [review]:
-----------------------------------------------------------------

A few issues mostly due to python versions.  We have been asking for months to upgrade the python versions on the talos machines.  In general we have always been able to work around any limitations, maybe we can include a zlib.py file or something like that.t
To run on try server:
1) python create_talos_zip.py
2) scp talos.{rev}.zip people.mozilla.org account
3) edit your m-c tree testing/talos/talos.json to point to the url on your people account for talos.{rev}.zip
4) push to try with the appropriate flags: try: -b o -p linux,macosx64,win32 -u none -t other

::: talos/ttest.py
@@ +245,5 @@
>          utils.setEnvironmentVars(browser_config['env'])
>          utils.setEnvironmentVars({'MOZ_CRASHREPORTER_NO_REPORT': '1'})
>  
> +        sps_profile_file = None
> +        if test_config['spsProfile'] == True and browser_config['remote'] == False:

nit:
if test_config['spsProfile'] and not browser_config['remote']:

@@ +410,5 @@
>                      time.sleep(browser_config['browser_wait'])
>  
> +                if test_config['spsProfile'] == True and browser_config['remote'] == False:
> +                    import base64
> +                    import zlib

we need to ensure base64 and zlib are available on all talos boxes.  Windows still runs on python 2.4 and we have different versions of python for each os :(  I don't believe zlib is available.

Try server can help confirm this.

@@ +411,5 @@
>  
> +                if test_config['spsProfile'] == True and browser_config['remote'] == False:
> +                    import base64
> +                    import zlib
> +                    with open(sps_profile_file, 'r') as fin:

'with' will not work on python 2.4, we need to do:
fin = open(sps_profile_file, 'r')
...
fin.close()

Comment 3

[(no longer active)]

Note that we need to publicize this on dev.platform before landing it, as it will most likely appear as a regression in Ts across the board.

Comment 4

[Benoit Girard (:BenWa)]

(In reply to Joel Maher (:jmaher) from comment #2)
> Comment on attachment 747758 [details] [diff] [review]
> Talos
> 
> Review of attachment 747758 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A few issues mostly due to python versions.  We have been asking for months
> to upgrade the python versions on the talos machines.  In general we have
> always been able to work around any limitations, maybe we can include a
> zlib.py file or something like that.t
> To run on try server:
> 1) python create_talos_zip.py
> 2) scp talos.{rev}.zip people.mozilla.org account
> 3) edit your m-c tree testing/talos/talos.json to point to the url on your
> people account for talos.{rev}.zip
> 4) push to try with the appropriate flags: try: -b o -p linux,macosx64,win32
> -u none -t other

https://tbpl.mozilla.org/?tree=Try&rev=285bf70f8db0

Comment 5

[Benoit Girard (:BenWa)]

It seems to work but is buggy. Linux seems to hang. I think I should disable it even for Ts and land this.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

I would be fine landing this for osx/windows.  lets make sure we only allow this for ts as the original patch had.  Going the route of a specific talos.zip (patch folks could use) to run on try server seems like a safe route to go.

Comment 7

[Benoit Girard (:BenWa)]

(In reply to Joel Maher (:jmaher) from comment #6)
> I would be fine landing this for osx/windows.  lets make sure we only allow
> this for ts as the original patch had.  Going the route of a specific
> talos.zip (patch folks could use) to run on try server seems like a safe
> route to go.

Right now the profile output is getting mixed with stderr quite often on mac. How about I default this to false everywhere but land this in Talos.

I discussed this issue with Ehsan and we decided that we should instead focusing on building something that developers can run locally rather then recording a ton of profiles that don't get looked at. Our default profiling options will likely miss useful info so I rather focus on building something that developers can tweak and bisect locally.

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

I am confused by your statement, it sounds like you don't want to run talos in production in anyway shape or form, but make it run locally.  Even when it runs in talos, you get results mixed up with stderr.  Shouldn't we add this feature in to work great for a local developer and not care about running in tinderbox automation?

Comment 9

[Benoit Girard (:BenWa)]

(In reply to Joel Maher (:jmaher) from comment #8)
> I am confused by your statement, it sounds like you don't want to run talos
> in production in anyway shape or form, but make it run locally.  Even when
> it runs in talos, you get results mixed up with stderr.  Shouldn't we add
> this feature in to work great for a local developer and not care about
> running in tinderbox automation?

Yes this is what I am suggesting. We should land it in Talos off by default and let local developers flip it on if they want to collect profiles. This way we don't have to worry about profile upload and storage.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

please update the patch to ignore linux and to output to a local file instead of intermixed with stdout/stderr.

I am fine with landing this and making it off by default!

Comment 11

[(no longer active)]

(In reply to comment #9)
> (In reply to Joel Maher (:jmaher) from comment #8)
> > I am confused by your statement, it sounds like you don't want to run talos
> > in production in anyway shape or form, but make it run locally.  Even when
> > it runs in talos, you get results mixed up with stderr.  Shouldn't we add
> > this feature in to work great for a local developer and not care about
> > running in tinderbox automation?
> 
> Yes this is what I am suggesting. We should land it in Talos off by default and
> let local developers flip it on if they want to collect profiles. This way we
> don't have to worry about profile upload and storage.

Or make it possible to be do that on the try server.

Comment 12

[Benoit Girard (:BenWa)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #11)
> (In reply to comment #9)
> > (In reply to Joel Maher (:jmaher) from comment #8)
> > > I am confused by your statement, it sounds like you don't want to run talos
> > > in production in anyway shape or form, but make it run locally.  Even when
> > > it runs in talos, you get results mixed up with stderr.  Shouldn't we add
> > > this feature in to work great for a local developer and not care about
> > > running in tinderbox automation?
> > 
> > Yes this is what I am suggesting. We should land it in Talos off by default and
> > let local developers flip it on if they want to collect profiles. This way we
> > don't have to worry about profile upload and storage.
> 
> Or make it possible to be do that on the try server.

It will be possible by pushing a custom talos.zip but you will be subjected to the log limit and will have to output to the console and deal with stdout/stderr mixing.

Comment 13

[Benoit Girard (:BenWa)]

Attachment: patch

This saves to a temporary file but also outputs it to the console which I think is useful if someone wants to run this on Talos.

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

I think the patch didn't upload or you have a blank patch.

Comment 15

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 756147 [details] [diff] [review]
patch

Review of attachment 756147 [details] [diff] [review]:
-----------------------------------------------------------------

with the nits addressed below, this is a nice simple patch.

::: talos/ttest.py
@@ +248,5 @@
> +        sps_profile = test_config.get('sps_profile', False)
> +        sps_profile_file = None
> +        if sps_profile and not browser_config['remote']:
> +            sps_profile_file = tempfile.mktemp();
> +            print "Activating Gecko Profiling. Temp. profile: " + sps_profile_file

I would prefer a utils.info() call here:
utils.info("Activating Gecko Profiling. Temp. profile: %s", sps_profile_file)

@@ +409,5 @@
>                  # on remote devices we do not have the fast launch/shutdown as we do on desktop
>                  if not browser_config['remote']:
>                      time.sleep(browser_config['browser_wait'])
>  
> +                if sps_profile == True and browser_config['remote'] == False:

if sps_profile and not browser_config['remote']:

@@ +411,5 @@
>                      time.sleep(browser_config['browser_wait'])
>  
> +                if sps_profile == True and browser_config['remote'] == False:
> +                    import base64
> +                    import zlib

are we sure zlib is available in python 2.4 and windows?  I think we answered this before, but I want to make sure.  We can test on try to know the truth.

@@ +414,5 @@
> +                    import base64
> +                    import zlib
> +                    fin = open(sps_profile_file, 'r')
> +                    print "Begin SPS Profile:\n"
> +                    print "data:text/x-sps_profile;base64,"

again, lets use utils.info here