1137339 - [manifestparser] Implement a chunk_by_runtime algorithm as a filter

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Andrew Halberstadt [:ahal]]

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.

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: 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

Comment 2

[Andrew Halberstadt [:ahal]]

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.

Comment 3

[Andrew Halberstadt [:ahal]]

s/manifest and its tests/manifest and assigning its tests

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 5

[Andrew Halberstadt [:ahal]]

(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.

Comment 6

[Andrew Halberstadt [:ahal]]

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 7

[Joel Maher ( :jmaher ) (UTC -8)]

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

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

Ship It!

Comment 8

[Andrew Halberstadt [:ahal]]

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

Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/86e026f0d539

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/86e026f0d539

Comment 11

[Andrew Halberstadt [:ahal]]

Attachment: MozReview Request: Bug 1137339 - [manifestparser] implement a chunk_by_runtime filter, r=jmaher

Comment 12

[Andrew Halberstadt [:ahal]]

Attachment: MozReview Request: Address review comments