Closed Bug 1210503 Opened 5 years ago Closed 5 years ago

perfherder compare view needs to show counters

Categories

(Tree Management :: Perfherder, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jmaher, Assigned: sabergeass)

References

Details

Attachments

(1 file)

47 bytes, text/x-github-pull-request
wlach
: review+
jmaher
: feedback+
Details | Review
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.
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.
Attached file 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!
Assignee: nobody → sabergeass
Status: NEW → ASSIGNED
Attachment #8674084 - Flags: feedback?(jmaher)
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.
Attachment #8674084 - Flags: feedback?(jmaher) → feedback+
(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.
yes, the compare view will need to be updated :)
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. :)
Flags: needinfo?(sabergeass)
Duplicate of this bug: 1193629
(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.
(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
Flags: needinfo?(sabergeass)
Attachment #8674084 - Flags: review?(wlachance)
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`
Attachment #8674084 - Flags: review?(wlachance) → review+
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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.
Duplicate of this bug: 1186938
You need to log in before you can comment on or make changes to this bug.