Closed Bug 1183877 Opened 5 years ago Closed 4 years ago

Increase total-chunks for Android 4.3 Debug crashtests, js-reftests, and reftests

Categories

(Release Engineering :: General, defect)

defect
Not set

Tracking

(firefox43 fixed)

RESOLVED FIXED
Tracking Status
firefox43 --- fixed

People

(Reporter: gbrown, Assigned: kmoir)

References

Details

Attachments

(8 files, 6 obsolete files)

13.56 KB, patch
jlund
: review+
kmoir
: checked-in+
Details | Diff | Splinter Review
28.21 KB, patch
jlund
: review+
Details | Diff | Splinter Review
70.31 KB, text/plain
Details
28.19 KB, patch
kmoir
: checked-in+
Details | Diff | Splinter Review
8.78 KB, patch
gbrown
: review+
kmoir
: checked-in+
Details | Diff | Splinter Review
2.45 KB, patch
jlund
: review+
kmoir
: checked-in-
Details | Diff | Splinter Review
3.68 KB, patch
jlund
: review+
kmoir
: checked-in+
Details | Diff | Splinter Review
56.66 KB, patch
Details | Diff | Splinter Review
See bug 1140471 and particularly https://bugzilla.mozilla.org/show_bug.cgi?id=1140471#c14:

"We think there is value in running Debug reftests on Android and we think the run-times are not surprising in context (we expect debug to run 2x to 3x slower, Android tests are slow, the emulator is slow and limits our ability to use multi-core cpu effectively).

"Let's try running these with as many chunks as necessary."
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cadc2b2dbc7&exclusion_profile=false hacks an increase in total-chunks as follows:

4 crashtest chunks
20 js-reftest chunks
48 plain-reftest chunks

Because of the nature of the hack, only the first few jobs of each category are run, but these results suggest that these total-chunk options are approximately correct.

Notice run-times from try:

C1 50 minutes
C2 55
J1 59
J2 50
J3 44
J4 53
J5 39
J6 55
R1 50
R2 47
R3 53
R4 67
R5 44
R6 35
...
Assignee: nobody → kmoir
Trying to think of a non-ugly hack to implement this.  It's a bit different in that we will have more chunks on debug that on opt. And I adding 48 test suites for debug plain reftests in our configs is not pretty.
Have some patches in progress for this and the the total # of builders doesn't seem to be a problem anymore :-) thanks to to jgriffin

gbrown: I assume you want these new tests to ride the trains?
Flags: needinfo?(gbrown)
That's great! Yes, please ride the trains.
Flags: needinfo?(gbrown)
Attached patch bug1183877.patch (obsolete) — Splinter Review
buildbot configs patch
Attached file bug1183877builder.diff (obsolete) —
builder diff
Depends on: 1148401
So I'm still working on this. It was quite a monumental task to get this working in staging but I think I have it working now and am waiting for test results.
Attached patch bug1183877m-c.patch (obsolete) — Splinter Review
Attachment #8640079 - Attachment is obsolete: true
mozilla-central patch for mozharness
Attachment #8641883 - Attachment is obsolete: true
Attached patch bug1183877.patch (obsolete) — Splinter Review
buildbot configs
Attachment #8643180 - Flags: review?(jlund)
Attachment #8643199 - Flags: review?(jlund)
I'm sorry kim I did not get to this today. I will look at it tomorrow
Comment on attachment 8643199 [details] [diff] [review]
bug1183877.patch

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

looks great! just one question below regarding try/cedar.

reseting r? for now. please r? me again when you have responded or updated the patch.

::: mozilla-tests/mobile_config.py
@@ +3002,5 @@
> +        'blob_upload': True,
> +        'timeout': 2400,
> +        'script_maxtime': 14400,
> +    },
> +    ),   

whitespace

@@ +3413,5 @@
>              BRANCHES[name]['platforms']['android-api-11']['panda_android']['debug_unittest_suites'] = deepcopy(ANDROID_MOZHARNESS_JSREFTEST + ANDROID_MOZHARNESS_CRASHTEST + ANDROID_MOZHARNESS_PLAIN_REFTEST)
>  
> +# bug 118387 Increase total-chunks for Android 4.3 Debug crashtests, js-reftests, and reftests
> +for name, branch in items_at_least(BRANCHES, 'gecko_version', 42):
> +    if name in ('try', 'cedar' ):

It seems like try and cedar have similar builder lists to other trunk branches wrt api 11+ debug. How come you are not changing these too?
Attachment #8643199 - Flags: review?(jlund)
Attachment #8643180 - Flags: review?(jlund) → review+
Attached patch bug1183877-2.patch (obsolete) — Splinter Review
Attachment #8643199 - Attachment is obsolete: true
Attached patch bug1183877builder-2.diff (obsolete) — Splinter Review
Attachment #8640083 - Attachment is obsolete: true
Attachment #8644424 - Attachment is obsolete: true
Attachment #8644426 - Attachment is obsolete: true
Comment on attachment 8644427 [details] [diff] [review]
bug1183877-3.patch

change it so new debug builders run on cedar and try too.  As well, leave 4.0 jobs running on try so they can still be run on try only they ride off the trains
Attachment #8644427 - Flags: review?(jlund)
Comment on attachment 8644427 [details] [diff] [review]
bug1183877-3.patch

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

thumbs!

fyi - I didn't put this second patch through dump master but logic wise it looks great

::: mozilla-tests/mobile_config.py
@@ +3412,5 @@
>              }
>              BRANCHES[name]['platforms']['android-api-11']['panda_android']['debug_unittest_suites'] = deepcopy(ANDROID_MOZHARNESS_JSREFTEST + ANDROID_MOZHARNESS_CRASHTEST + ANDROID_MOZHARNESS_PLAIN_REFTEST)
>  
> +# bug 1183877 Increase total-chunks for Android 4.3 Debug crashtests, js-reftests, and reftests
> +for name, branch in items_at_least(BRANCHES, 'gecko_version', 42):    

nit: whitespace

@@ +3428,5 @@
> +                'opt_unittest_suites': deepcopy(ANDROID_4_3_C3_DICT['opt_unittest_suites']),
> +                'debug_unittest_suites': deepcopy(ANDROID_4_3_C3_TRUNK_DICT['debug_unittest_suites'] + ANDROID_4_3_MOZHARNESS_DEBUG_TRUNK),}
> +            if name in ('try'):
> +                continue
> +            BRANCHES[name]['platforms']['android-api-11']['panda_android']['debug_unittest_suites'] = []

I wonder if this will bite us later down the line. e.g. if we accidentally added a line below this and didn't see the try continue block above.

maybe we should say something like:

if name != 'try':
     BRANCHES[name]['platforms']['android-api-11']['panda_android']['debug_unittest_suites'] = []
Attachment #8644427 - Flags: review?(jlund) → review+
Comment on attachment 8643180 [details] [diff] [review]
bug1183877m-c.patch

pushed to m-i
Attachment #8643180 - Flags: checked-in+
Blocks: 1193002
buildbot patch hasn't landed yet so reopening
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
patch with review comments incorporated
also changed gecko version to 43 since merge has occurred
Attachment #8646932 - Flags: checked-in+
Attachment #8646983 - Flags: review?(gbrown)
Attachment #8646983 - Flags: review?(gbrown) → review+
Comment on attachment 8646983 [details] [diff] [review]
1183877bustage.patch

earlier today
Attachment #8646983 - Flags: checked-in+
I had this patch om my staging master but missed attaching it when it came to review time. Thus the cause of the (non-capacity) failures today.
Attachment #8647126 - Flags: review?(jlund)
Comment on attachment 8643180 [details] [diff] [review]
bug1183877m-c.patch

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

::: testing/mozharness/configs/android/androidarm_4_3.py
@@ +306,5 @@
>                  "tests/layout/reftests/reftest.list"]
>          },
> +        "reftest-17": {
> +            "category": "reftest",
> +            "extra args": ["--total-chunks=48", "--this-chunk=17",

I'm kicking myself for missing this. I take half the blame.
Comment on attachment 8647126 [details] [diff] [review]
bug1183877chunking.patch

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

I can't wait to axe this complexity now that we have all of mh in tree. these in_tree bits should go away
Attachment #8647126 - Flags: review?(jlund) → review+
Thanks Jordan for the reviews.  I shoulder the blame for the errors - my apologies.

So aside from the errors in my patches there are two issues from yesterday's burning trees that I need to address before enabling this again. We are adding 48 jobs on every commit.  This causes capacity issues
1) We don't have a large enough emulator64 pool to handle this
2) We don't have SETA data on these new tests to reduce the number of tests that are run on each commit.

So the next steps are that I'm going to enable these tests on a project branch to ensure the latest patches fix the issues. Then I'll talk to jmaher about how to get SETA data for these tests in a faster time frame than the usual month.
Attachment #8647126 - Flags: checked-in+
Comment on attachment 8647126 [details] [diff] [review]
bug1183877chunking.patch

backed out as gbrown pointed out this will may opt tests run fewer tests

from irc
gbrown	I'm worried that 4.3 opt reftests (etc) will think total-chunks are increased but only run chunks {1..16} of 48
gbrown	no failures, but lots of tests missing
gbrown	I think we may need reftest + reftest-debug sections in the mozharness config...messy
Attachment #8647126 - Flags: checked-in+ → checked-in-
Had a meeting wrt SETA today and decided the way forward for this was to enable the tests on try (hidden), run tests n times, then extrapolate this data to m-i etc so we can enable on other branches without crippling wait times.

https://etherpad.mozilla.org/increasing-seta
Depends on: 1201236
Depends on: 1204756
Once we have data from SETA from bug 1205526 in place, we can enable the 4.3 debug tests on all branches and disable the corresponding 4.0 ones. Luckily, we are still under the builder limit on the linux64 masters.
builder diff
Attachment #8674926 - Flags: review?(jlund)
Attachment #8674926 - Flags: review?(jlund) → review+
Attachment #8674926 - Flags: checked-in+
in production
I can see jobs being skipped according to SETA data, so I'm going to close this bug.  We still have Android 4.0 debug jobs running on Pandas m-a, m-b, m-r and talos on all branches.  Talked to jmaher in #ateam and he suggested we could tackle m-a/m-b/m-r in a week or so.
Status: REOPENED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → FIXED
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.