1210427 - compare view to pick revisions could aid developers with some simple changes

Status: RESOLVED FIXED

(Tree Management :: Perfherder, defect)

Description

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

if a there is a random revision from a few days ago a developer might want to investigate the performance results.  We did great stuff on the test chooser, including the option for a drop down of recent commits.  in many cases we are looking at commits from 3+ days old, treeherder doesn't load well and it is really difficult to find the revision-1.

Here is what would be nice:
* enter a revision into compare chooser
* upon entering it a popup comes up some choices:
** a list of dates/branches that the revision is seen on, ideally you will pick the earliest date which would use the tip which was pushed for that revision
** a checkbox which upon selection would populate the base revision as the previous tip.  Alternatively, we could make this optional and show a list of the selected branch/tip from the first choice and the 3 revision tips prior/after in a list for selection to populate the base

this would make it a simpler interface to select our base/new revisions.  For revisions where new is on the try branch, we should not do this.

Other considerations would be dealing with revisions that have no future data/merges (a branch new commit) which you want to generate a compare view to look at 6 hours later.

Comment 1

[Avi Halachmi (:avih)]

Thanks, Joel.

I'm restating the goal in more general terms:

1. A user wants to know the effect of a specific patch on performance.
2. The user knows the m-i or m-c changeset which includes the patch.

The goal, given the changset from #2:

3. Know what to enter into compareperf:
  - new: the earliest changeset which was tested (tip?) which includes the patch.
  - old: the latest changeset which was tested which doesn't include the patch.

4. Possibly, once the user knows the two changesets from #3 above, the user would also like to retrigger some of the jobs on these revisions.

#4 above could be as simple as providing links to the specific changesets on treeherder (once we have #3), and from there jobs could be retriggered.

Or, as a future enhancements, allow to do N retriggers of the jobs for these two changesets directly from compareperf.

Comment 2

[Julien Pagès (:parkouss)]

Attachment: find prev revision/branch

Hey guys!

Here is my first attempt for this - only proposal for the branch/revision now (no automatic triggering). Looks good to me, but you need to triple check all this; The logic for try must be verified, not sure how (but I guess Joel have an idea for that!).

So, choosing a new branch/revision should popup a link, allowing to autofill the base. Tell me what do you think.

Comment 3

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

Comment on attachment 8713613 [details] [review]
find prev revision/branch

this seems like a lot of code, but you are solving a lot of scenarios!  I made a few comments in the PR- some might be me not fully understanding or being over careful.  

Most of the complexities are dealing with finding the parent revision, and it is fairly easy to follow.  As for doing things most optimally or the right treeherder way, I will let Will comment on that :)

This is great to see!

Comment 4

[Julien Pagès (:parkouss)]

I fixed most of the things with the new commit:

https://github.com/parkouss/treeherder/commit/e9744be09b092412c14e8028d556004dfd191d7d

You can have a look to see the diffs. One or two questions in the PR still needs attention - I can squash everything at the end.

Thanks for the feedbacks!

Comment 5

[William Lachance (:wlach)]

Comment on attachment 8713613 [details] [review]
find prev revision/branch

This is great! Nearly ready to land, it looks like.

Comment 6

[Julien Pagès (:parkouss)]

Comment on attachment 8713613 [details] [review]
find prev revision/branch

Ok, I just committed one last thing, to use the branches listed in value.js (look like we agreed on this).

https://github.com/parkouss/treeherder/commit/d4fb65541371faf7541e68fc5372e30d76fcacc1

It would be great to test that at least one try and one other branch (m-c or m-i) are working as expected for you - I think :wlach tried something with success but not sure if the two cases are validated.

Once everything is good, I can squash the commits, and ask for landing!

I don't think two reviews are required now, the first who tackle the final review can clear the other request I believe;

Last thing, I propose that we consider auto triggering in another bug, it might need some discussion and the code is probably not related. What do you think ?

Comment 7

[William Lachance (:wlach)]

(In reply to Julien Pagès (:parkouss) from comment #6)
> Comment on attachment 8713613 [details] [review]
> find prev revision/branch
> 
> Ok, I just committed one last thing, to use the branches listed in value.js
> (look like we agreed on this).
> 
> https://github.com/parkouss/treeherder/commit/
> d4fb65541371faf7541e68fc5372e30d76fcacc1
> 
> It would be great to test that at least one try and one other branch (m-c or
> m-i) are working as expected for you - I think :wlach tried something with
> success but not sure if the two cases are validated.

Just validated that m-i works as expected. Had some trouble with some other try revisions I just attempted:

https://treeherder.allizom.org/#/jobs?repo=try&revision=d00e579c9a88
https://treeherder.allizom.org/#/jobs?repo=try&revision=4be8044a4ff6

These are some results :mconley pushed today, didn't have time to look at what they're based on. It would be good to figure out why they didn't work.

Also, if you click on a particular talos job in treeherder, you'll see a link saying "Compare results against other revision" (currently links to treeherder.mozilla.org always, but that's a seperate problem, I'll file a bug). If I follow that link, it doesn't seem to automatically check to see if there's a baseline. It would be great if it did.

Here's an example of a selected job (against stage)

https://treeherder.allizom.org/#/jobs?repo=try&revision=d00e579c9a88&selectedJob=16674255
https://treeherder.allizom.org/perf.html#/comparechooser?newProject=try&newRevision=d00e579c9a88

> Once everything is good, I can squash the commits, and ask for landing!
> 
> I don't think two reviews are required now, the first who tackle the final
> review can clear the other request I believe;
> 
> Last thing, I propose that we consider auto triggering in another bug, it
> might need some discussion and the code is probably not related. What do you
> think ?

Yes, we should do auto-(re)triggering from the compare view, but agree that it's out of scope for this bug. Let's get this landed first. :)

Comment 8

[William Lachance (:wlach)]

(In reply to William Lachance (:wlach) from comment #7)

> Also, if you click on a particular talos job in treeherder, you'll see a
> link saying "Compare results against other revision" (currently links to
> treeherder.mozilla.org always, but that's a seperate problem, I'll file a
> bug). 

See bug 1244367

Comment 9

[William Lachance (:wlach)]

Comment on attachment 8713613 [details] [review]
find prev revision/branch

I'm probably not going to have time to do a final review on this before I head to retreat. This is definitely going in the right direction, but there are some things that should be investigated and there is one use case which is unaddressed.

I'll leave the r? with :jmaher. This will be a huge timer saver when it lands. Thanks for doing this!!

Comment 10

[Julien Pagès (:parkouss)]

Thanks Will, this is great information.

So yeah, I was not sure about try, sounds like there is something wrong. I would like to discuss with you how you manually look for a baseline given a try revision.

Comment 11

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

parkouss, I can help you out with the manual root cause finding.

Comment 12

[William Lachance (:wlach)]

(In reply to Julien Pagès (:parkouss) from comment #10)
> Thanks Will, this is great information.
> 
> So yeah, I was not sure about try, sounds like there is something wrong. I
> would like to discuss with you how you manually look for a baseline given a
> try revision.

It's not too difficult to find try revisions to test -- just open the try repository on treeherder and filter on "talos". Hopefully jmaher can help you with any details while I'm away. :)

Comment 13

[Julien Pagès (:parkouss)]

(In reply to William Lachance (:wlach) [Away Jan 30 - Feb 6 2016] from comment #12)
> (In reply to Julien Pagès (:parkouss) from comment #10)
> > Thanks Will, this is great information.
> > 
> > So yeah, I was not sure about try, sounds like there is something wrong. I
> > would like to discuss with you how you manually look for a baseline given a
> > try revision.
> 
> It's not too difficult to find try revisions to test -- just open the try
> repository on treeherder and filter on "talos". Hopefully jmaher can help
> you with any details while I'm away. :)

Yeah, well I'm mostly interested on how usually one can find the base branch revision starting from a given try revision - not finding the try revision itself. :)

(In reply to Joel Maher (:jmaher) from comment #11)
> parkouss, I can help you out with the manual root cause finding.

Thanks! Let's discuss about that on irc then.

Comment 14

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

Comment on attachment 8713613 [details] [review]
find prev revision/branch

lets ship this thing.

Comment 15

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/182f4cb426522bb09499ac2905c632f5c6d554d9
Bug 1210427 - Find the base line for a revision

In the perfherder compare view, selecting the new branch and revision
now search for the base line of that revision (e.g. the first branch and
revision available to compare to).

If the base line is found, the user can click a link to automatically
fill the base branch and revision.

https://github.com/mozilla/treeherder/commit/7bc64866144041c3578b0d5c5bc579a4c1e43cba
Merge pull request #1283 from parkouss/bug-1210427

Bug 1210427 - Find the base line for a revision