809276 - GetXRes should parse output of xrestop in process versus crazy shell commands

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Jeff Hammel]

Boo:
http://hg.mozilla.org/build/talos/file/0316cd82e0a4/talos/cmanager_linux.py#l63

Instead, we should read the output of xrestop and parse it with one of
the better languages in the world for parsing: the very python process
that we are using.

Comment 1

[Jeff Hammel]

Attachment: programmatically read xrestop counters

Not quite in final form but it works.  This returns a dict of xrestop output keyed on the PID

Comment 2

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

Comment on attachment 679007 [details] [diff] [review]
programmatically read xrestop counters

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

1) it would be really nice if you included in a comment what the example output looks like.
2) I really really don't like to add a new file for a linux specific counter which I assume nobody tracks.
3) how can we be certain that we are running the correct version of xrestop on the specific machine

this is a good start and moves us closer to less bash hacks.

::: talos/xrestop.py
@@ +17,5 @@
> +def xrestop():
> +    args = ['-m', '1', '-b']
> +    process = subprocess.Popen(['xrestop'] + list(args),
> +                               stdout=subprocess.PIPE, stderr=subprocess.PIPE)
> +    stdout, stderr = process.communicate()

we never use stderr, no need to define it.

@@ +19,5 @@
> +    process = subprocess.Popen(['xrestop'] + list(args),
> +                               stdout=subprocess.PIPE, stderr=subprocess.PIPE)
> +    stdout, stderr = process.communicate()
> +    if process.returncode:
> +        raise NotImplementedError

in talos, we probably want to make this a tbpl friendly error.

Comment 3

[Jeff Hammel]

(In reply to Joel Maher (:jmaher) from comment #2)
> Comment on attachment 679007 [details] [diff] [review]
> programmatically read xrestop counters
> 
> Review of attachment 679007 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 1) it would be really nice if you included in a comment what the example
> output looks like.

Will do.

> 2) I really really don't like to add a new file for a linux specific counter
> which I assume nobody tracks.

If its genuinely not used we should probably turn it off.  I can add this to utils.py, if you'd rather. FWIW, xperf has its own directory and several files.

> 3) how can we be certain that we are running the correct version of xrestop
> on the specific machine

This behaviour isn't changed with this patch.  Both before and after whatever version is on $PATH is used.  If we want to make this configurable, that can be done, but I'd prefer that in a follow up bug.

> this is a good start and moves us closer to less bash hacks.
> 
> ::: talos/xrestop.py
> @@ +17,5 @@
> > +def xrestop():
> > +    args = ['-m', '1', '-b']
> > +    process = subprocess.Popen(['xrestop'] + list(args),
> > +                               stdout=subprocess.PIPE, stderr=subprocess.PIPE)
> > +    stdout, stderr = process.communicate()
> 
> we never use stderr, no need to define it.

I can call this '_' or what not, but process.communicate() returns the tuple (stdout, stderr) so I have to assign it to something

> @@ +19,5 @@
> > +    process = subprocess.Popen(['xrestop'] + list(args),
> > +                               stdout=subprocess.PIPE, stderr=subprocess.PIPE)
> > +    stdout, stderr = process.communicate()
> > +    if process.returncode:
> > +        raise NotImplementedError
> 
> in talos, we probably want to make this a tbpl friendly error.

I was planning on raising a generic exception, as I'm not sure what exactly this should be.  The calling code can turn this into a talosError

Comment 4

[Jeff Hammel]

>> in talos, we probably want to make this a tbpl friendly error.

>I was planning on raising a generic exception, as I'm not sure what exactly this should be.  The calling code can turn this into a talosError

Looking at the code currently, we actually don't raise a talosError.  Should we?

Comment 5

[Jeff Hammel]

Attachment: proposed implementation

Comment 6

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

Comment on attachment 679270 [details] [diff] [review]
proposed implementation

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

even though this creates a new file and quadruples the amount of code it is replacing I am still going to r+.  This is more redundant and provides us with greater flexibility by using the xrestop toolchain if we so desire.  Also this has tests!

Comment 7

[Jeff Hammel]

 cmdline = "xrestop -m 1 -b | grep -A 15 " + str(pid) + " | tr -d \"\n\" | sed \"s/.*total bytes.*: ~//g\""

It is worth noting that the existing way of doing things, in particular grepping for the PID, can result in false positives.  The patch should fix this.  Since (ABICT) we ultimately swallow exceptions from counters these would never be noticed

Comment 8

[Phil Ringnalda (:philor)]

The dozens of times a day I star failed runs as bugs with "unable to proceed with missing counter" in the summary makes me wonder about the accuracy of "never be noticed."

Comment 9

[Jeff Hammel]

Yeah, sorry, I'm an idiot.  These *do* fail since http://hg.mozilla.org/build/talos/rev/524c6ff1736b

Comment 10

[Mozilla RelEng Bot]

Try run for 4085436fa6f7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4085436fa6f7
Results (out of 7 total builds):
    success: 1
    failure: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4085436fa6f7

Comment 11

[Jeff Hammel]

Fixing this...it turns out coding while feverish is a bad idea.

Talos is currently borken as per https://bugzilla.mozilla.org/show_bug.cgi?id=811016 ; i'll probably take that along with this since i need it.

Comment 12

[Jeff Hammel]

Attachment: update with fixes

carrying r=jmaher forward

Comment 13

[Jeff Hammel]

trying on linux, again: https://tbpl.mozilla.org/?tree=Try&rev=7aa372c29cd3

Comment 14

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #13)
> trying on linux, again: https://tbpl.mozilla.org/?tree=Try&rev=7aa372c29cd3

This looks green.  :jmaher, I'm going to hold off pushing pending your approval.  Feel free to push yourself, though

Comment 15

[Mozilla RelEng Bot]

Try run for 7aa372c29cd3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=7aa372c29cd3
Results (out of 7 total builds):
    success: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-7aa372c29cd3

Comment 16

[Jeff Hammel]

pushed: http://hg.mozilla.org/build/talos/rev/a950457156dc