692812 - On mozilla-aurora and mozilla-beta run only PGO builds rather than Non-PGO + periodic PGO

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Description

[Armen [:armenzg]]

This would save 16 sets of testing jobs (every 4 hours by 2 branches).
This would help us reduce wait times.

This is actually quite important.

Comment 1

[[e-mail] (:bhearsum)]

This is something that needs to go out to a wider audience. I doubt we can only run PGO builds on these branches. I would say that we have to at least have them on mozilla-central as well, and maybe mozilla-inbound. Also, bug 692796 should take a away some of the pain you mention.

Comment 2

[(not currently active) Ted Mielczarek]

I think he means "make all builds on mozilla-aurora and mozilla-beta PGO builds", instead of the non-PGO + periodic PGO builds that we're doing on mozilla-central.

Comment 3

[Armen [:armenzg]]

What ted says. Sorry.

I do suggest some other scheduling improvements in bug 692823.

This would cut around 640 tests per branch (8 sets of PGO sts by ~20 of suites per 4 PGO enabled testing OSes) and 24 builds per branch (8 scheduled PGO builds by 3 PGO enabled platforms).

Comment 4

[Armen [:armenzg]]

Attachment: [not tested] disable periodic PGO builder for m-a, m-b and m-r

jhford what is the right way to fix this?
My patch only disables the PGO periodic builder but I don't know how to make the opt build to be PGO since the in-source mozconfigs (on next merge day) will not have PGO specified. 

jhford what do you think of this?
diff --git a/misc.py b/misc.py
--- a/misc.py
+++ b/misc.py
@@ -1105,8 +1105,12 @@ def generateBranchObjects(config, name):
             if name in ('mozilla-1.9.1', 'mozilla-1.9.2', 'mozilla-2.0'):
                 # We force profiledBuild off here because its meaning has changed
                 # We deal with turning on PGO for these old branches in the actual factor
                 factory_kwargs['profiledBuild'] = False
+            if name in ('mozilla-aurora', 'mozilla-beta', 'mozilla-release',):
+                # For m-a, m-b and m-r we want opt builds to be PGO
+                # With in-tree mozconfigs is difficult to set this up
+                factory_kwargs['profiledBuild'] = True
 
             mozilla2_dep_factory = factory_class(**factory_kwargs)
             mozilla2_dep_builder = {
                 'name': '%s build' % pf['base_name'],

Comment 5

[Armen [:armenzg]]

Attachment: [b-c] disable PGO periodic builds and make the opt builds to be PGO back again

Comment 6

[Armen [:armenzg]]

Attachment: [bcustom] turn dep builds into PGO builds

Comment 7

[Armen [:armenzg]]

Attachment: differences after my patch

- pgo builders are removed
- opt builds will have MOZ_PGO=1
- opt builds will trigger pgo-talos instead of opt-talos
- opt builds will trigger opt-unittest instead of pgo-unittest

This is not perfect but it does the job.

What else is needed for perfect?
* get rid of opt-talos builders since they are not going to be triggered
** only pgo-talos will be triggered
* opt builds should trigger pgo-unittest
** on tbpl we would see unit tests listed under the "opt" row rather than "pgo" row until this is addressed
** after this change we should also get rid of opt-unittest

To accomplish this we would need to do a lot of refactoring and/or even turn pgo into a type of build rather than shoehorn it into misc.py.

We can file a bug to address the remaining issues and I can volunteer if wanted.

Comment 8

[Chris Cooper [:coop] (he/him)]

Comment on attachment 567092 [details] [diff] [review]
[bcustom] turn dep builds into PGO builds

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

::: misc.py
@@ +1108,5 @@
>  
>              if name in ('mozilla-1.9.1', 'mozilla-1.9.2', 'mozilla-2.0'):
>                  # We force profiledBuild off here because its meaning has changed
>                  # We deal with turning on PGO for these old branches in the actual factory
>                  factory_kwargs['profiledBuild'] = False

We already special-case profiledBuild for other branches. Why not just disable PGO builds in the configs for these branches and then set profiledBuild here, rather than adding yet another pgo flag to set?

Comment 9

[Chris Cooper [:coop] (he/him)]

Comment on attachment 567091 [details] [diff] [review]
[b-c] disable PGO periodic builds and make the opt builds to be PGO back again

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

You have no default value set for make_dep_build_to_be_pgo.

See my comment on the buildbotcustom patch.

Comment 10

[Chris Cooper [:coop] (he/him)]

Armen expressed some concern over adding more special-case branch code to buildbotcustom. This won't get rid of the special-case branch code that's already in there, but it is a valid concern.

Maybe it's just the chosen pref name that didn't sit well with me. Really, we want something that indicates that there will *only* be PGO builds for this branch, and it will override enable_pgo and not add the extra -PGO flagging.

I would be happy with a pref named "only_pgo" or "always_pgo" that had a suitable default value in GLOBAL_VARS (set to False), and then the corresponding changes to buildbotcustom to work with that. Make sense?

Comment 11

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

(In reply to Chris Cooper [:coop] from comment #10)
> Armen expressed some concern over adding more special-case branch code to
> buildbotcustom. This won't get rid of the special-case branch code that's
> already in there, but it is a valid concern.
> 
> Maybe it's just the chosen pref name that didn't sit well with me. Really,
> we want something that indicates that there will *only* be PGO builds for
> this branch, and it will override enable_pgo and not add the extra -PGO
> flagging.
> 
> I would be happy with a pref named "only_pgo" or "always_pgo" that had a
> suitable default value in GLOBAL_VARS (set to False), and then the
> corresponding changes to buildbotcustom to work with that. Make sense?

What about having 'enable_pgo' control whether PGO is on for a branch at all, and have 'pgo_strategy' as another key.  We could set the strategy to 'periodic' for the separate 'pgo' builds and 'per-checkin' for every build on the branch being PGO enabled.

Fwiw, i'd also rather not having more branch special casing.  The 1.9.2 special casing was done because its a legacy branch and it is totally different, as far as enabling PGO goes.

Comment 12

[Justin Wood (:Callek)]

(In reply to John Ford [:jhford] from comment #11)
> (In reply to Chris Cooper [:coop] from comment #10)
> > Armen expressed some concern over adding more special-case branch code to
> > buildbotcustom. This won't get rid of the special-case branch code that's
> > already in there, but it is a valid concern.
> > 
> > Maybe it's just the chosen pref name that didn't sit well with me. Really,
> > we want something that indicates that there will *only* be PGO builds for
> > this branch, and it will override enable_pgo and not add the extra -PGO
> > flagging.
> > 
> > I would be happy with a pref named "only_pgo" or "always_pgo" that had a
> > suitable default value in GLOBAL_VARS (set to False), and then the
> > corresponding changes to buildbotcustom to work with that. Make sense?
> 
> What about having 'enable_pgo' control whether PGO is on for a branch at
> all, and have 'pgo_strategy' as another key.  We could set the strategy to
> 'periodic' for the separate 'pgo' builds and 'per-checkin' for every build
> on the branch being PGO enabled.
> 
> Fwiw, i'd also rather not having more branch special casing.  The 1.9.2
> special casing was done because its a legacy branch and it is totally
> different, as far as enabling PGO goes.

If we go with strategy, my opinion is to drop enable_pgo entirely, and for "No PGO at all" we just set pgo_strategy to None. allowing future strategy changes if need be, and of course if there is a pgo_strategy, then we have pgo enabled!

Comment 13

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

(In reply to Justin Wood (:Callek) from comment #12)
> If we go with strategy, my opinion is to drop enable_pgo entirely, and for
> "No PGO at all" we just set pgo_strategy to None. allowing future strategy
> changes if need be, and of course if there is a pgo_strategy, then we have
> pgo enabled!

I think I agree!

Armen, mind if I steal this bug to implement comment 11 and maybe comment 12?

Comment 14

[Armen [:armenzg]]

You don't have to ask me twice :) Let me know if you want reviews from me.

Comment 15

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

Attachment: buildbot-configs v1

this patch is the configuration portion of the change

Comment 16

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

Attachment: buildbotcustom v1

This is the logic required for this to work

Comment 17

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

Attachment: differences between dumpmaster runs before and after

Comment 18

[Armen [:armenzg]]

Comment on attachment 571216 [details] [diff] [review]
buildbotcustom v1

I agree with your comment to rename pgoBuilders. r+ with changing it to periodicPgoBuilders.

Thanks John!

Comment 19

[Armen [:armenzg]]

Comment on attachment 571215 [details] [diff] [review]
buildbot-configs v1

This got checked-in last week.

Comment 20

[Armen [:armenzg]]

This went live at 8:45 AM PDT.