670037 - Need to differentiate between pgo builds and non-pgo builds

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[John Ford [:jhford] CET/CEST Berlin Time]

once bug 658313 lands, we will have 'mini-nightly' builds every four hours.  We will also have the option to have PGO tests on try.

Builds with 'pgo-build' are dep builds with PGO on and those with 'build' are dep builds with PGO off.

sample strings that are involved for builds are:

Linux mozilla-central pgo-build
Linux mozilla-central build
WINNT 5.2 mozilla-central pgo-build
WINNT 5.2 mozilla-central build

I haven't yet determined what the testing string will look like.

Comment 1

[Phil Ringnalda (:philor)]

IDing types of jobs happens in http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/file/3606b746b7b5/js/Data.js#l509 if you want to pick names based on that.

However, if we're doing both a PGO and a non-PGO build on the same rev, we're probably better off treating PGO as a separate OS with a separate row (well, three separate rows (well, four, Linux-PGO, Linux64-PGO, Win7-PGO, WinXP-PGO)), so it'll mostly be a matter of #l479 picking out the separate OS and pgo- strings and gluing them into the "OS" Linux-PGO (with the usual fun of having the Windows build be on 2K3 but we want to pretend it was on Win7 which we call generic Win to line it up with those tests).

Oh, and for bonus bonus fun, you'll be stuck behind bug 669000, so you'll probably have to deploy your change by manually patching tbpl.m.o :)

Comment 2

[John Ford [:jhford] CET/CEST Berlin Time]

What is your eta for fixing this?

Comment 3

[Robert Helmer [:rhelmer]]

(In reply to John Ford [:jhford] from comment #2)
> What is your eta for fixing this?

I can work on this next week, does the approach in comment 1 sound reasonable to everyone (treating PGO the same way separate OSes are, if I am understanding correctly?)

Comment 4

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Robert Helmer [:rhelmer] from comment #3)
> (In reply to John Ford [:jhford] from comment #2)
> > What is your eta for fixing this?
> 
> I can work on this next week, does the approach in comment 1 sound
> reasonable to everyone (treating PGO the same way separate OSes are, if I am
> understanding correctly?)

great!

That sounds reasonable to me.

The unit tests are going to use the format

<platform> <branch> pgo test <suite>

<platform> <branch> pgo talos <suite>

I am not sure if the property will bubble up to what is accessible by tbpl, but I have

+        if self.profiledBuild:
+            sendchange_props['pgo_build'] = True
+        else:
+            sendchange_props['pgo_build'] = False

added to the sendchange properties.

Comment 5

[Markus Stange [:mstange]]

(In reply to Robert Helmer [:rhelmer] from comment #3)
> I can work on this next week, does the approach in comment 1 sound
> reasonable to everyone (treating PGO the same way separate OSes are, if I am
> understanding correctly?)

No objections from my side.

Comment 6

[John Ford [:jhford] CET/CEST Berlin Time]

Does anyone know how the current tbpl is going to look if it gets these results before the changes in this bug are made?

Comment 7

[Phil Ringnalda (:philor)]

If you only add pgo, so that the job has the exact same name plus pgo, then it will display a second build and a second test in the same row, "B B M(1 1 2 2 3 3 4 4 5 5) etc.", and it will be possible to tell them apart by clicking on each and looking down in the lower left corner to see "Rev3 Fedora 12 mozilla-central opt test mochitests-2/5" versus "Rev3 Fedora 12 mozilla-central opt pgo test mochitests-2/5"

Comment 8

[John Ford [:jhford] CET/CEST Berlin Time]

Ok, it sounds like it is ideal for this to be fixed before the PGO automation changes land, but it isn't a strict requirement.  Does that sound reasonable?

Comment 9

[Phil Ringnalda (:philor)]

Attachment: fix v.1

Wow, it's been a long time since we added a row. Hope this is really all that's required - it works fine without having any pgo-named builds in the data, anyway.

The 120px thing is unfortunate, but it's going to be someone other than me who fixes the need to stretch that column out to fit the longest name so I don't have a weird wide gap in onlyunstarred=1 with only a "Win opt        oth" showing.

The only outcome worth stressing over is "omg tbpl's completely broken, we have to close the tree," which I'm six 9's sure won't happen without this patch, and four 9's sure won't happen with it, so probably the best thing to do is land this so it's on tbpl-dev, make sure cshields and rhelmer will be around Tuesday afternoon, then once the reconfig has happened and there's a PGO build and a PGO test and tbpl-dev is still fine, deploy (or, when the first PGO build breaks prod but not dev, deploy in a desperate hurry, but that ain't gonna happen).

Comment 10

[Arpad Borsos [:Swatinem]]

Comment on attachment 563996 [details] [diff] [review]
fix v.1

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

So do we have PGO for both debug and opt builds? I thought it was just opt?
Looking at http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/file/e6b69bb53a1b/js/UserInterface.js#l1070 again, we might want to put opt into type (thats what we call opt/debug) instead of os.

Comment 11

[Phil Ringnalda (:philor)]

Well, one thing's for sure: after figuring out that we have machines, which have a type property that's xpcshell or nightly or mochitest, and a debug property that's true or not, and pushes, which have a type array that's debug or opt, we need to call opt/debug/pgo something other than type. My ibuprofen bottle's not full enough for that sort of thing.

Comment 12

[Phil Ringnalda (:philor)]

Attachment: fix v.2

But you're right, this is considerably prettier (both in the code, and not having the all-caps acronym shouting out PGO! PGO! all over the place), and my headache will fade with time.

Comment 13

[Arpad Borsos [:Swatinem]]

Comment on attachment 564069 [details] [diff] [review]
fix v.2

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

Very nice, I like the general cleanup.

::: js/UserInterface.js
@@ +1075,1 @@
>               (push.results[os].debug ? self._buildHTMLForOS(os, " debug", push.results[os].debug, order) : '');

This can probably be simplified with a similar flavors array as above, but leave it for later.

Comment 14

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/53156392a7ce

Comment 15

[Phil Ringnalda (:philor)]

And http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/8f19c6e6af28 because I saw that I was going to be making tooltips like "XPCShellTestdebug was successful" but I thought for a while about whether I wanted to just put the space directly in the flavor, and then I made a sandwich, and then something something something else.

Comment 16

[John Ford [:jhford] CET/CEST Berlin Time]

How is this going to make it into production?  Should I be asking for this to be deployed during my downtime?

Comment 17

[Phil Ringnalda (:philor)]

Nah, no point in deploying it during, since the time when we'll know any more about whether I broke the world is "after the first pgo build finishes, or really, after the first pgo test finishes on Windows" so you're looking at somewhere around 18:00 or 20:00, probably. I'll just file to get it deployed now, and hope I don't turn out to have broken something then.