move the sps profilling stuff in another module

RESOLVED FIXED

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: parkouss, Assigned: parkouss)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Right now the sps profiling feature is coded into the TTest._runTest method.

We should move that code in his own file, to clearly separate concerns.

See the discussion in bug 1190270 around this.
(Assignee)

Comment 1

3 years ago
Created attachment 8643314 [details] [diff] [review]
sps_profile.patch

This move the sps_profile code in his own module.

Works good locally, with and without --spsProfile option.

pushed a try without sps here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6dbe8cf64c09

Maybe we should push one try with sps also ?
Assignee: nobody → j.parkouss
Status: NEW → ASSIGNED
Attachment #8643314 - Flags: review?(jmaher)
Comment on attachment 8643314 [details] [diff] [review]
sps_profile.patch

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

I like this patch in general, but I don't like the:
from talos import utils
from talos.utils import TalosError
from talos.sps_profile import SpsProfile

I didn't think we had talos as a module?
Attachment #8643314 - Flags: review?(jmaher) → review-
(Assignee)

Comment 3

3 years ago
We have now, with bug 1189021. :)
I forgot!  How will this work locally?
(Assignee)

Comment 5

3 years ago
Just fine, since talos is installed (so the package talos exists locally), and "import talos" already worked there.

This module thing was only an issue with harness, since we are not installing talos - but the dependencies on harness. But this has been fixed, so we can use real nice imports now.
does this imply that you run talos from a specific directory?  I think it is ok, but I want to confirm before changing to a r+.

Also have you done a push to try with --spsProfiling enabled?
(Assignee)

Comment 7

3 years ago
(In reply to Joel Maher (:jmaher) from comment #6)
> does this imply that you run talos from a specific directory?  I think it is
> ok, but I want to confirm before changing to a r+.

Well, no, this is not required. Not for me at least, as I use the talos 'binary' that is provided. Also no just if the user uses a virtualenv (like what is done using INSTALL.py). So I think this is safe from that point of view.

> 
> Also have you done a push to try with --spsProfiling enabled?

Not yet! I was wondering what would be the appropriate syntax. Maybe exactly the one you provided in bug 1190270 ? Also the try here is pretty unreadable, because of the reasons explained by mail. The fix in bug 1190907 should do the trick for now, so I am waiting for it to be landed before repushing to try here - I think a bit with and whithout --spsProfiling (two trys).
(Assignee)

Comment 8

3 years ago
Created attachment 8644844 [details] [diff] [review]
sps_profile.patch

Asking again for review here. Same patch as before, just a bit more of clean up.

This is working file locally (missingsymbols.zip and profile_tresize.sps.zip are created when using --spsProfile and that MOZ_UPLOAD_DIR is defined), I'll push to try with sps_profile later.
Attachment #8643314 - Attachment is obsolete: true
Attachment #8644844 - Flags: review?(jmaher)
Attachment #8644844 - Flags: review?(jmaher) → review+
(Assignee)

Comment 10

3 years ago
Oh, I tried to build the syntax manually for spsProfile, seems like a failure. Canceled, and repushed to try (using trychooser this time):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb12c8fea1af
(Assignee)

Comment 11

3 years ago
Hmm, looks pretty good but I'm a bit worried with the two red jobs in:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb12c8fea1af

Does not look related to my change to me, but maybe I should try again the job without my change to be sure ?
I would try again, it could have been a bad base you started with.
(Assignee)

Comment 14

3 years ago
Joel, I have a bunch of errors that seems unrelated:

send failed, graph server says:
11:20:19     INFO -  RETURN:to determine geomean from 'test_run_values' for  50795710 - local variable 'values' referenced before assignment
11:20:19     INFO -  RETURN:  File "/var/www/html/graphs/server/pyfomatic/collect.py", line 295, in handleRequest
11:20:19     INFO -  RETURN:    geomean = valuesReader(databaseCursor, databaseModule, inputStream, metadata)
11:20:19     INFO -  RETURN:  File "/var/www/html/graphs/server/pyfomatic/collect.py", line 238, in valuesReader
11:20:19     INFO -  RETURN:    raise DatabaseException("to determine geomean from 'test_run_values' for  %s - %s" % (metadata.test_run_id, str(x)))
11:20:19     INFO -  RETURN:
11:20:19     INFO -  RETURN:

Also I still have two win8 talos chrome busted. Will retry those without my patch to be sure.
(Assignee)

Comment 15

3 years ago
So tried without my patch, one busted, the other green. Will put back my patch an try again some more.
(Assignee)

Comment 16

3 years ago
put pack my changes, on 4 jobs two busted, two success. It does not seems related to my change. I think we are good landing this, but maybe we should investigate what is this intermittent bug first ?

As you want Joel - I would go to land that patch, since this does not seems related an that it only appears when the spsProfile option is activated on try (I suspect I tried again on a bad base as you suggested). Then we should try again with spsProfile, and maybe fill a bug.
lets land the patch.  I suspect this is a specific test causing troubles.
(Assignee)

Comment 18

3 years ago
Landed:

https://hg.mozilla.org/build/talos/rev/e8854f96ade3
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.