Closed Bug 1137339 Opened 6 years ago Closed 6 years ago

[manifestparser] Implement a chunk_by_runtime algorithm as a filter

Categories

(Testing :: Mozbase, defect)

defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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.
Attached file MozReview Request: bz://1137339/ahal (obsolete) —
/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)
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.
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)
(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.
Attachment #8570016 - Flags: review?(jmaher)
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: nobody → ahalberstadt
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/86e026f0d539
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Attachment #8570016 - Attachment is obsolete: true
Attachment #8619603 - Flags: review+
Attachment #8619604 - Flags: review+
You need to log in before you can comment on or make changes to this bug.