1160078 - With --persist, prioritize already downloaded builds over builds which require a download

Status: RESOLVED FIXED

(Testing :: mozregression, enhancement)

Description

[Elbart]

When doing a regression-search, I often encounter a situation where mozregression downloads a Nightly-build, although builds which are only a couple of days older/younger are already available in the persist-folder.

When the known regression-window is bigger than, let's say, two weeks, one or two days difference probably won't matter much regression-wise, but it would save a probably unnecessary download.

Comment 1

[Julien Pagès (:parkouss)]

I think this is a great idea. :)

The numbers you give (2 days difference for 2 weeks minimum) seems good to me. More generally, maybe we could do a simple division by 7 of the date range to get the number of days around we could use ?

Range   day range (older/younger) that we can use
7        1
14       2
21       3
28       4
35       5
42       6
49       7
56       8
63       9
70       10
77       11
84       12
91       13
98       14
105      15
112      16
119      17
126      18
133      19

Comment 2

[Julien Pagès (:parkouss)]

Will, do you think we should make this the default behavior (when --persist is used) ?

Comment 3

[William Lachance (:wlach)]

(In reply to Julien Pagès from comment #2)
> Will, do you think we should make this the default behavior (when --persist
> is used) ?

sure, makes sense to me

Comment 4

[Julien Pagès (:parkouss)]

Attachment: With --persist, prioritize already downloaded builds over builds which require a download

So this add a new behaviour to reuse *nearest* nightly build files when using persistent mode as specified in previous comments.

Note this can be disabled (cmdline or configuration) with the *approx-policy* flag.

I tested that locally with success, but this is quite a change and should be double checked. :)

Also I did not implement that for inbounds. Still it is possible, but this needs to be discussed (I added a comment about that in the patch). So we could open a follow up bug, or discuss here about the issue (and if we want this for inbound). Basically, what currently stops me is that we order inbound revs using push time. If we order then using the mercurial push id, it would be possible for me to build a map chset -> push id and be able to know what is the previous/next changeset of the one we want to test, thus allowing to implement the same kind of logic.

I like the idea of ordering with push id (since it is the real order of the changes!) but I can understand that coming from nightly bisection one can hope a time-based order. Though most time the order must be exactly the same.

Comment 5

[William Lachance (:wlach)]

Comment on attachment 8636488 [details] [review]
With --persist, prioritize already downloaded builds over builds which require a download

I don't really have time to properly review this right now. :( I'm going to leave this to your judgement. If it works well for you, I'd say it's ok to push.

A general comment: Although I think in principle stuff like this makes sense, we probably need to set a limit on the amount of additional things we add to mozregression: at this point, we have a lot of features that (I suspect) only a very small number of people use and have a real cost in terms of making it more difficult to understand and test.

Comment 6

[Julien Pagès (:parkouss)]

(In reply to William Lachance (:wlach) from comment #5)
> A general comment: Although I think in principle stuff like this makes
> sense, we probably need to set a limit on the amount of additional things we
> add to mozregression: at this point, we have a lot of features that (I
> suspect) only a very small number of people use and have a real cost in
> terms of making it more difficult to understand and test.

Makes sense.

Comment 7

[jonathan.pigree]

Okay. I made a review on your Julien's PR. I have commented the parts that seems odd to me.

Then I tested it rapidly:
-> With the gui, it does not work, but if I recall correctly, the link with the GUI isn't done yet
-> It worked in command line on a simple test

I let myself a little more days to test it more thoroughly though. Then I think it will be okay.

PS: I rebased your branch approx on the actual master HEAD without conflicts before testing.

Comment 8

[Julien Pagès (:parkouss)]

Thanks a lot for that thorough review Jonathan!

I will fix or comment the things you mentioned, and ask then for a final review.

Yes, it is normal that the GUI does not use that. It won't be hard to add it, but we should add a way to configure that feature (I am thinking about just use/not use it), and preferably activated by default (same as with the cmdline tool). This is for a follow up bug!

Comment 9

[jonathan.pigree]

Seems great to me. Works smoothly.

Comment 10

[jonathan.pigree]

Comes very handy. I mean it will save us a lot of time and bandwith. Awesome.

Comment 11

[Julien Pagès (:parkouss)]

Thanks a lot Jonathan for the work here!

I think we did enough testing on this - I merged it as:

https://github.com/mozilla/mozregression/commit/dcb827c38cd85751eade7043aba1da018334b9e0

Comment 12

[Julien Pagès (:parkouss)]

Well, I reverted the changes with https://github.com/mozilla/mozregression/commit/943e8f49d43ae408be53fe3fe208ea0e5baab6fe, because of 1194616.

This needs some more work (see bug 1194616).

Comment 14

[jonathan.pigree]

Sorry. My review and tests were not complete enough.

Comment 17

[Julien Pagès (:parkouss)]

Attachment: approx builds

This works fine from my testing. Note this is a command line only feature for now. (I'll tackle the GUI later)

Comment 18

[William Lachance (:wlach)]

Comment on attachment 8709036 [details] [review]
approx builds

I didn't look too closely, but the overall idea sounds reasonable and the benefits of the feature for people with slow connections are obvious!

Comment 19

[Julien Pagès (:parkouss)]

Merged in https://github.com/mozilla/mozregression/commit/aabe9a6219f8bfe71d30a476ee396d99e1a34554, along with some tests, and the integration in the GUI.

This looks all good to me, I made it the default behavior for both cmdline and GUI. We'll see how it goes in live!