971926 - Requesting OMTC Reftest (Ro) variant for Windows

Status: RESOLVED WONTFIX

(Release Engineering :: General, defect, P2)

Description

[Benoit Girard (:BenWa)]

The graphics team is working on releasing OMTC for windows. We're not quite ready to switch over but the test are passing and we're ready to begin testing automation for this new configuration. In a few releases when we switch over to running OMTC by default on windows this job can be removed (As the current R job will match this one).

This should work exactly like the Ru job except that instead of preference it set, it should set:
layers.offmainthreadcomposition.enabled;true

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Releng, how do we make sure this happens quickly, before OMTC deteriorates because it isn't getting tested?

Comment 2

[Phil Ringnalda (:philor)]

For reasons probably lost in the mists of time, "exactly like the Ru job" means "opt only, not debug; Win7 and Win8 and Win64 but not WinXP" - is that what you want for this as well?

Comment 3

[Benoit Girard (:BenWa)]

I had not noticed that. Preferably both configuration. *If* we have to chose here we should chose Debug to catch assertions.

Comment 4

[Phil Ringnalda (:philor)]

And does it apply to WinXP, or not?

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

It does apply to XP.  If there are resource issues, XP+Win7 debug as the minimum.

Comment 6

[Benoit Girard (:BenWa)]

Can we get an assignee for this? Regressions are expensive to chase down and fix when we don't catch them in time.

Comment 7

[Benoit Girard (:BenWa)]

ping

Comment 8

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

Assigning to Armen for now. He will let me know if that's a mistake.

Comment 9

[Armen [:armenzg]]

I will hope to look into this at the end of this week.

Comment 10

[Armen [:armenzg]]

Attachment: wip add reftets-omtc to Cedar

I'm trying to look for a volunteer for this and here's my assessment.

We run both reftests and reftest-no-accel [1]
We want to add something similar to reftest-no-accel but for OMTC; let's call it reftest-omtc.

We need to make these changes:
* enable reftest-omtc on buildbot-configs for Cedar
** once working we can determine on which other branches to run it
* add the definition to mozharness (upcoming patch)
* file a bug for tbpl to show the job nicely
* once we're satisfied with the results on Cedar, we can look into enabling it on other branches


[1] https://tbpl.mozilla.org/?jobname=reftest-no-accel&showall=1

Comment 11

[Armen [:armenzg]]

Attachment: reftest-omtc definition for omtc

I see that philor already filed the tbpl bug.

BenWA, is the pref properly set on this patch?

Comment 12

[Jordan Lund (:jlund)]

Comment on attachment 8387595 [details] [diff] [review]
reftest-omtc definition for omtc

Syntax and placement looks good.

Benwa can say whether the options are right but he said in -- https://bugzilla.mozilla.org/show_bug.cgi?id=971926#c0

that this should behave the exact same as Ru jobs(no-accel) except set offmainthreadcomposition to true.

If that is the case, then I think we need the other prefs from "reftest-no-accel".

something like this: https://pastebin.mozilla.org/4506375

Comment 13

[Jordan Lund (:jlund)]

> that this should behave the exact same as Ru jobs(no-accel) except set
> offmainthreadcomposition to true.
> 
> If that is the case, then I think we need the other prefs from
> "reftest-no-accel".
> 

I think I read that wrong. "This should work exactly like the Ru job except that instead of preference it set, it should set"

so I think you were right. we should set offmainthreadcomposition *instead* of Ru prefs.

Comment 14

[Benoit Girard (:BenWa)]

Comment on attachment 8387595 [details] [diff] [review]
reftest-omtc definition for omtc

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

::: configs/unittests/win_unittest.py
@@ +103,5 @@
>                          '--setpref=browser.tabs.remote.autostart=true',
>                          'tests/reftest/tests/layout/reftests/reftest-sanity/reftest.list'],
>          "reftest-no-accel": ["--setpref=gfx.direct2d.disabled=true", "--setpref=layers.acceleration.disabled=true",
>                               "tests/reftest/tests/layout/reftests/reftest.list"],
> +        "reftest-omtc": ["--setpref=layers.offmainthreadcomposition.enabled=true",

This is correct. We do -not- want to change the value of layers.acceleration.disabled.

Comment 15

[Hal Wine [:hwine] use NI!]

Attachment: wip add reftets-omtc to Cedar

passes test-masters.sh

Comment 16

[Hal Wine [:hwine] use NI!]

Comment on attachment 8387595 [details] [diff] [review]
reftest-omtc definition for omtc

https://hg.mozilla.org/build/mozharness/rev/ddc959dd7774

Comment 17

[Jordan Lund (:jlund)]

Comment on attachment 8387814 [details] [diff] [review]
wip add reftets-omtc to Cedar

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

I think we will need to have a clear idea of what platforms we want or don't want before we can throw this into the wild (see inline review comments)

Also, I'm not sure how mozilla-tests/config.py works right now but it seems like we define affiliated script config files like so: http://mxr.mozilla.org/build/source/buildbot-configs/mozilla-tests/config.py#1087

It appears repetitive but if IIUC we will need a key like:
    'reftest-omtc': {'config_files': ["unittests/win_unittest.py"],},

inside every platform we wish to support in: PLATFORM_UNITTEST_VARS


I might be completely wrong here as I am unfamiliar so please correct me if I am way off :)

::: mozilla-tests/config.py
@@ +1753,5 @@
>  
>  ######## cedar
>  BRANCHES['cedar']['platforms']['macosx64']['mavericks']['opt_unittest_suites'] = UNITTEST_SUITES['opt_unittest_suites'][:]
>  BRANCHES['cedar']['platforms']['macosx64']['mavericks']['debug_unittest_suites'] = UNITTEST_SUITES['debug_unittest_suites'][:]
> +BRANCHES['cedar']['platforms']['win32']['xp-ix']['_unittest_suites'] = UNITTEST_SUITES['opt_unittest_suites'] + REFTEST_OMTC

looking at: https://bugzilla.mozilla.org/show_bug.cgi?id=971926#c2

reftest-no-accel does not run on xp. If we are sticking with the same platforms, we don't want reftest-omtc to right?

@@ +1758,5 @@
> +BRANCHES['cedar']['platforms']['win32']['win7-ix']['opt_unittest_suites'] = UNITTEST_SUITES['opt_unittest_suites'] + REFTEST_OMTC
> +BRANCHES['cedar']['platforms']['win32']['win8']['opt_unittest_suites'] = UNITTEST_SUITES['opt_unittest_suites'] + REFTEST_OMTC
> +BRANCHES['cedar']['platforms']['win32']['xp-ix']['opt_unittest_suites'] = UNITTEST_SUITES['debug_unittest_suites'] + REFTEST_OMTC
> +BRANCHES['cedar']['platforms']['win32']['win7-ix']['_unittest_suites'] = UNITTEST_SUITES['debug_unittest_suites'] + REFTEST_OMTC
> +BRANCHES['cedar']['platforms']['win32']['win8']['_unittest_suites'] = UNITTEST_SUITES['debug_unittest_suites'] + REFTEST_OMTC

so I think all these '_unittest_suites' are supposed to be '{opt, debug}_unittest_suites'


reftest-no-accel also seems to run on:
    - 'win8_64' -> http://mxr.mozilla.org/build/source/buildbot-configs/mozilla-tests/config.py#1196
    - 'win64_vm' -> http://mxr.mozilla.org/build/source/buildbot-configs/mozilla-tests/config.py#1268

Comment 18

[Hal Wine [:hwine] use NI!]

Attachment: wip add reftets-omtc to Cedar

Addresses earlier errors (not detected by test-masters.sh :(

As for which platforms this should run on, my understanding of comment 10 was that we'd get running on this minimal set first on cedar, then make decision on load impact. We'd add any additional platforms to cedar, then roll everything out to production.

Comment 19

[Jordan Lund (:jlund)]

Comment on attachment 8388585 [details] [diff] [review]
wip add reftets-omtc to Cedar

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

after reading https://bugzilla.mozilla.org/show_bug.cgi?id=971926#c18, platforms lgtm

see in line comments for minor concerns.

::: mozilla-tests/config.py
@@ +1096,5 @@
>                      'config_files': ["unittests/win_unittest.py"],
>                  },
> +                'reftest-omtc': {
> +                    'config_files': ["unittests/win_unittest.py"],
> +                },

I still think we will need to add this under every platform we wish to support.

eg: see how many times this comes up in config.py:
  'reftest-no-accel': {
       'config_files': ["unittests/win_unittest.py"],
  },

Looks like we are supporting three below: xp-ix, win7-ix, and win8

Maybe armen can confirm?

@@ +1756,5 @@
>  
>  ######## cedar
>  BRANCHES['cedar']['platforms']['macosx64']['mavericks']['opt_unittest_suites'] = UNITTEST_SUITES['opt_unittest_suites'][:]
>  BRANCHES['cedar']['platforms']['macosx64']['mavericks']['debug_unittest_suites'] = UNITTEST_SUITES['debug_unittest_suites'][:]
> +BRANCHES['cedar']['platforms']['win32']['xp-ix']['debug_unittest_suites'] = UNITTEST_SUITES['opt_unittest_suites'] + REFTEST_OMTC

I would imagine we can't (or at least shouldn't) match BRANCHES's'debug_unittest_suites' with UNITTEST_SUITES's 'opt_unittest_suites'

@@ +1759,5 @@
>  BRANCHES['cedar']['platforms']['macosx64']['mavericks']['debug_unittest_suites'] = UNITTEST_SUITES['debug_unittest_suites'][:]
> +BRANCHES['cedar']['platforms']['win32']['xp-ix']['debug_unittest_suites'] = UNITTEST_SUITES['opt_unittest_suites'] + REFTEST_OMTC
> +BRANCHES['cedar']['platforms']['win32']['win7-ix']['opt_unittest_suites'] = UNITTEST_SUITES['opt_unittest_suites'] + REFTEST_OMTC
> +BRANCHES['cedar']['platforms']['win32']['win8']['opt_unittest_suites'] = UNITTEST_SUITES['opt_unittest_suites'] + REFTEST_OMTC
> +BRANCHES['cedar']['platforms']['win32']['xp-ix']['opt_unittest_suites'] = UNITTEST_SUITES['debug_unittest_suites'] + REFTEST_OMTC

This assignment value could be swapped with xp-ix above

Comment 20

[Milan Sreckovic [:milan] (needinfo for best results)]

I can't tell if these are minor changes to be made or a full rethink, but ni on Hal to see what the next steps are.

Comment 21

[Hal Wine [:hwine] use NI!]

I'm not sure - I think there are multiple views of what's wanted. Let's get a reset on the needs to start.

As I understand it, the plan is to do a phased rollout:
 a) get this running on cedar for debug + op5 builds (comment 3) on win XP/7 & win8 platforms (comment 5) (4 cases)
 b) after test debugged, expand to more builds, and address any resource issues
 c) with no problems, roll out to main branches

Assuming I have that correctly, we should be able to handle (a) this week.

Comment 22

[Milan Sreckovic [:milan] (needinfo for best results)]

Awesome, (a) for this week sounds great!

Comment 23

[Hal Wine [:hwine] use NI!]

Attachment: wip add reftets-omtc to Cedar

This addresses all issues from comment 19

Comment 24

[Jordan Lund (:jlund)]

Comment on attachment 8393757 [details] [diff] [review]
wip add reftets-omtc to Cedar

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

I think we are close! Aside from the 'win8' key issue I commented on below, it turns out we have a hidden side affect.

I ran builder_list against this default tip and it turns out we would be adding marionette and cppunit additional builders to the cedar debug platforms that we added omtc to.

I guess UNITTEST_SUITES['debug_unittest_suites'] contains these builders and we never added them to cedar before. So we will want to know if: whether that was on purpose or if we should be running those builders on cedar.

Digging in mozilla-tests/config.py, it seems we already define things like BRANCHES['cedar']['platforms']['win32']['win8']['debug_unittest_suites'] here: http://mxr.mozilla.org/build/source/buildbot-configs/mozilla-tests/config.py#1143

That shows what suites should run for a given platform (notice it says marionette should only be run on try). So maybe we can just stick with that. In fact, we add metro chrome that way it seems: http://mxr.mozilla.org/build/source/buildbot-configs/mozilla-tests/config.py#1867

::: mozilla-tests/config.py
@@ +1798,5 @@
> +BRANCHES['cedar']['platforms']['win32']['win7-ix']['opt_unittest_suites'] = UNITTEST_SUITES['opt_unittest_suites'] + REFTEST_OMTC
> +BRANCHES['cedar']['platforms']['win32']['win8-ix']['opt_unittest_suites'] = UNITTEST_SUITES['opt_unittest_suites'] + REFTEST_OMTC
> +BRANCHES['cedar']['platforms']['win32']['xp-ix']['debug_unittest_suites'] = UNITTEST_SUITES['debug_unittest_suites'] + REFTEST_OMTC
> +BRANCHES['cedar']['platforms']['win32']['win7-ix']['debug_unittest_suites'] = UNITTEST_SUITES['debug_unittest_suites'] + REFTEST_OMTC
> +BRANCHES['cedar']['platforms']['win32']['win8-ix']['debug_unittest_suites'] = UNITTEST_SUITES['debug_unittest_suites'] + REFTEST_OMTC

I *think* it's win8 not win8-ix

make checkconfig confirms that it's not liking 'win8-ix'

Comment 25

[Jordan Lund (:jlund)]

Attachment: omtc-140319.diff

I was already knee deep in the configs after last review so I just tried out my theory from the last review comments.

This patch passes checkconfig and the diff of builder list can be seen here: https://pastebin.mozilla.org/4630208

what do you think? should we go with something like this?

Comment 26

[Hal Wine [:hwine] use NI!]

Comment on attachment 8393800 [details] [diff] [review]
omtc-140319.diff

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

yeah - that's what I meant to type -- THANKS

Comment 27

[Jordan Lund (:jlund)]

Comment on attachment 8393800 [details] [diff] [review]
omtc-140319.diff

cool, glad hopefully our brains combined have conquered buildbot-configs for one day

on 'default' -> https://hg.mozilla.org/build/buildbot-configs/rev/963af70f0a92

Comment 28

[Jordan Lund (:jlund)]

This buildbot patch is in production

And the results are in: OMTC suites were ran in Cedar but they did not pass. Here is a snippet of one platform:
https://tbpl.mozilla.org/php/getParsedLog.php?id=36465002&tree=Cedar

It's hard to tell whether cedar is in bad shape right now (many suites seem to be awfully orange) or if omtc needs some love.

Hopefully having this now on cedar helps you diagnose the root cause.

Next steps: as per comment 21, once (b) is complete, releng will roll out to more branches.

Comment 29

[Hal Wine [:hwine] use NI!]

BenWa -- when you are ready for us to take the next step, please unassign the bug from yourself.

Comment 30

[Benoit Girard (:BenWa)]

This is why I was hoping to get a quick turn around on this :(

Comment 31

[Benoit Girard (:BenWa)]

OMTC is turned on by default. This is no longer relevant.