1210503 - perfherder compare view needs to show counters

Status: RESOLVED FIXED

(Tree Management :: Perfherder, defect)

Description

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

right now when I get a regression on memory counters, I can view it in perfherder, but it never shows up in the compare view.

This should not be the case.  As it stands we only collect counters for tp5o and xperf.

Comment 1

[William Lachance (:wlach)]

We need to do the same thing in the compare front end that :MikeLing did in test_analyze_perf in bug 1186569: that is, include subtests which have no summary in the overall view.

Comment 2

[MikeLing]

Attachment: PR for bug 1210503

Hi Joel and Will,

I would like to work on this issue if it's ok :) 

As I recall, we alert none summary series in bug 1186569. So I just create a PR for this but *haven't test it*, Joel, could you tell me what's the result should I got if I make correct? Then I will address my PR and ask review for it. Thank you!

Comment 3

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

Comment on attachment 8674084 [details] [review]
PR for bug 1210503

this looks like the right direction.

We have counters (such as "tp5o main_rss") which have no subtests.  We just need to ensure all summaries and counters are displayed when we view a comparison.

One thing to watch out for is that xperf has many counters and that might clutter things up.  Counters are only for tp5o (all platforms) and xperf (windows 7).  Should we put the counters in another spot on the compare view?  probably not, but it should be easy to discover and not clutter it up too much.

Comment 4

[MikeLing]

(In reply to Joel Maher (:jmaher) from comment #3)
> Comment on attachment 8674084 [details] [review]
> PR for bug 1210503
> 
> this looks like the right direction.
> 
> We have counters (such as "tp5o main_rss") which have no subtests.  We just
> need to ensure all summaries and counters are displayed when we view a
> comparison.

Hmm, the other thing is maybe we need to update the compare view because not all the test we display has "subtest" now, so we could just hid that button if we are display a none summary test.

Comment 5

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

yes, the compare view will need to be updated :)

Comment 6

[William Lachance (:wlach)]

Hey Mike, are you going to have time to work on this soon? There's a bunch of work I want to do on the compare view, but I'd rather this land first. If you could give this priority out of the issues you're working on, it would help me a lot. Thanks. :)

Comment 8

[William Lachance (:wlach)]

(In reply to MikeLing from comment #4)
> (In reply to Joel Maher (:jmaher) from comment #3)
> > Comment on attachment 8674084 [details] [review]
> > PR for bug 1210503
> > 
> > this looks like the right direction.
> > 
> > We have counters (such as "tp5o main_rss") which have no subtests.  We just
> > need to ensure all summaries and counters are displayed when we view a
> > comparison.
> 
> Hmm, the other thing is maybe we need to update the compare view because not
> all the test we display has "subtest" now, so we could just hid that button
> if we are display a none summary test.

By the way, this sounds like the right to do to me.

Comment 9

[MikeLing]

(In reply to William Lachance (:wlach) from comment #6)
> Hey Mike, are you going to have time to work on this soon? There's a bunch
> of work I want to do on the compare view, but I'd rather this land first. If
> you could give this priority out of the issues you're working on, it would
> help me a lot. Thanks. :)

Sure thing!I will put this as top priority and commit a pr in these two days

Comment 10

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/748e38f883437d68869a5af8e2f3d324790da577
Bug 1210503 - Include non-summary tests in main comparison view

Comment 11

[William Lachance (:wlach)]

Comment on attachment 8674084 [details] [review]
PR for bug 1210503

This looks great! The only thing I changed was the commit message, to: `Bug 1210503 - Include non-summary tests in main comparison view`

Comment 12

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/4b499e4fde93004039be2120beb8dd78b3206c14
Bug 1210503 - Fix bad merge

Lost changes in bug 1187000 when this was merged the first time, so e10s
results were not visible.