If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[manifestparser] Implement a chunk_by_runtime algorithm as a filter

RESOLVED FIXED in Firefox 39

Status

Testing
Mozbase
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: ahal, Assigned: ahal)

Tracking

(Blocks: 1 bug)

unspecified
mozilla39
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
We'd like to start chunking suites based on the average runtimes of each individual tests such that each chunk takes approximately the same amount of time.
(Assignee)

Comment 1

3 years ago
Created attachment 8570016 [details]
MozReview Request: bz://1137339/ahal

/r/4371 - Bug 1137339 - [manifestparser] implement a chunk_by_runtime filter, r=jmaher

Pull down this commit:

hg pull review -r b305dc4f2985e74ec9cbdbf51f0026d9c2e7b577
Attachment #8570016 - Flags: review?(jmaher)
(Assignee)

Comment 2

3 years ago
This algorithm works by repeatedly taking the longest running manifest and its tests to the (current) shortest running chunk. On each iteration the shortest chunk is recalculated. This seems to give good results while still keeping tests of the same manifest together.
(Assignee)

Comment 3

3 years ago
s/manifest and its tests/manifest and assigning its tests
Comment on attachment 8570016 [details]
MozReview Request: bz://1137339/ahal

https://reviewboard.mozilla.org/r/4369/#review3579

::: testing/mozbase/manifestparser/manifestparser/filters.py
(Diff revision 1)
> +        self.runtimes.update(runtimes)

why do we do a defaultdict(int), then update(runtimes)?  Does this take care of the case where runtimes in None or {} ?

::: testing/mozbase/manifestparser/manifestparser/filters.py
(Diff revision 1)
> +        self.total = total

I wish this was called total_chunks

::: testing/mozbase/manifestparser/manifestparser/filters.py
(Diff revision 1)
> +        self.this = this

this is confusing when you are familiar with other languages, can we call this current_chunk or this_chunk?

::: testing/mozbase/manifestparser/manifestparser/filters.py
(Diff revision 1)
> +            tests_by_chunk[0][1].extend(batch)

why are we only modifying tests_by_chunk[0] ?  Is there a chance that the .sort() we do sorts it and we keep putting the next highest values in the chunks with the lowest runtime (at a start it is value of 0)?

If I got that, it would be better to make the code a bit more self reading.

::: testing/mozbase/manifestparser/tests/test_chunking.py
(Diff revision 1)
> +                     { <dir>: <num tests>

this is clear, but it should be a complete json object in the example.
Attachment #8570016 - Flags: review?(jmaher)
(Assignee)

Comment 5

3 years ago
(In reply to Joel Maher (:jmaher) from comment #4)
> why do we do a defaultdict(int), then update(runtimes)?  Does this take care
> of the case where runtimes in None or {} ?

No, defaultdict(int) means any keys that don't exist in the dictionary will default to a value of 0 if accessed. E.g:
d = defaultdict(int)
d['foo'] == 0 # True

The idea is that tests that don't exist in the runtimes file are so quick that their runtimes essentially count for nothing. The tool that generates the runtimes files would decide whether to include negligible tests or not.
 
> why are we only modifying tests_by_chunk[0] ?  Is there a chance that the
> .sort() we do sorts it and we keep putting the next highest values in the
> chunks with the lowest runtime (at a start it is value of 0)?
> 
> If I got that, it would be better to make the code a bit more self reading.

Yes every iteration tests_by_chunk gets re-sorted so tests_by_chunk[0] is always the fastest chunk. I'm not sure how to make this more self reading, but I can add a comment.
(Assignee)

Updated

3 years ago
Attachment #8570016 - Flags: review?(jmaher)
(Assignee)

Comment 6

3 years ago
Comment on attachment 8570016 [details]
MozReview Request: bz://1137339/ahal

/r/4371 - Bug 1137339 - [manifestparser] implement a chunk_by_runtime filter, r=jmaher
/r/4719 - Address review comments

Pull down these commits:

hg pull review -r 6b4fdf41f64eae071a61b1e99169255e334ca959
Comment on attachment 8570016 [details]
MozReview Request: bz://1137339/ahal

https://reviewboard.mozilla.org/r/4369/#review3863

Ship It!
Attachment #8570016 - Flags: review?(jmaher) → review+
(Assignee)

Updated

3 years ago
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
(Assignee)

Comment 8

3 years ago
The test passes on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a292ce1f520

Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/86e026f0d539
https://hg.mozilla.org/mozilla-central/rev/86e026f0d539
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Assignee)

Comment 10

2 years ago
Comment on attachment 8570016 [details]
MozReview Request: bz://1137339/ahal
Attachment #8570016 - Attachment is obsolete: true
Attachment #8619603 - Flags: review+
Attachment #8619604 - Flags: review+
(Assignee)

Comment 11

2 years ago
Created attachment 8619603 [details]
MozReview Request: Bug 1137339 - [manifestparser] implement a chunk_by_runtime filter, r=jmaher
(Assignee)

Comment 12

2 years ago
Created attachment 8619604 [details]
MozReview Request: Address review comments
You need to log in before you can comment on or make changes to this bug.