Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12090088:
Add Chrome Endure graphs in the overview of the perf dashboard. (Closed)
|
Created:
7 years, 10 months ago by Dai Mikurube (NOT FULLTIME) Modified:
7 years, 10 months ago CC:
chromium-reviews Base URL:
https://git.chromium.org/git/chromium/tools/perf.git@master Visibility:
Public. |
DescriptionAdd Chrome Endure graphs in the overview of the perf dashboard.
BUG=chromium-os:32302
NOTRY=true
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=180842
Patch Set 1 #
Total comments: 7
Patch Set 2 : addressed the comments. #
Total comments: 2
Messages
Total messages: 14 (0 generated)
Hi Mike, Chase and Dennis, Could you take a look? This is the grand sum of the changes to support Chrome Endure in the current perf dashboard. It expects that the following changes has been already landed: * https://codereview.chromium.org/12094074/ * https://codereview.chromium.org/12087097/
2 nits and a question https://codereview.chromium.org/12090088/diff/1/dashboard/overview.html File dashboard/overview.html (right): https://codereview.chromium.org/12090088/diff/1/dashboard/overview.html#newco... dashboard/overview.html:10: history = 50 add semicolon https://codereview.chromium.org/12090088/diff/1/dashboard/overview.html#newco... dashboard/overview.html:13: document.write('history=' + history) add semicolon https://codereview.chromium.org/12090088/diff/1/dashboard/overview.html#newco... dashboard/overview.html:297: <script>DisplayGraph("endure-linux-dbg/gmail-testGmailComposeDiscard", "GmailComposeDiscard-l0-DMP", 0);</script> how did you decide upon this set of graphs to display on the overview page?
Thanks for the comments, Dennis. https://codereview.chromium.org/12090088/diff/1/dashboard/overview.html File dashboard/overview.html (right): https://codereview.chromium.org/12090088/diff/1/dashboard/overview.html#newco... dashboard/overview.html:10: history = 50 On 2013/02/01 01:33:35, dennis_jeffrey wrote: > add semicolon Done. https://codereview.chromium.org/12090088/diff/1/dashboard/overview.html#newco... dashboard/overview.html:13: document.write('history=' + history) On 2013/02/01 01:33:35, dennis_jeffrey wrote: > add semicolon Done. https://codereview.chromium.org/12090088/diff/1/dashboard/overview.html#newco... dashboard/overview.html:297: <script>DisplayGraph("endure-linux-dbg/gmail-testGmailComposeDiscard", "GmailComposeDiscard-l0-DMP", 0);</script> On 2013/02/01 01:33:35, dennis_jeffrey wrote: > how did you decide upon this set of graphs to display on the overview page? I chose the number, four, since the overview.html should not have so many pages. The reason why I picked them up is to have: * both release and debug * both pure-benchmarking and application (gmail) It's not so clear reason. Please tell me your suggestions if you have. Also, I think about adding overview-endure.html which is similar to the page generated by endure_result_parser.py.
gentle ping...
General l gtm with one question. I'm deferring to Dennis or Chase here for the final approval. https://chromiumcodereview.appspot.com/12090088/diff/5001/dashboard/overview.... File dashboard/overview.html (right): https://chromiumcodereview.appspot.com/12090088/diff/5001/dashboard/overview.... dashboard/overview.html:294: <script>DisplayGraph("endure-linux-rel/control-testControlAttachDetachDOMTree", "ControlAttachDetachDOMTree-TabMem-Private", 0);</script> I noticed that none of the other calls to DisplayGraph include the graph argument. Is it needed here, and has it been tested?
https://chromiumcodereview.appspot.com/12090088/diff/1/dashboard/overview.html File dashboard/overview.html (right): https://chromiumcodereview.appspot.com/12090088/diff/1/dashboard/overview.htm... dashboard/overview.html:297: <script>DisplayGraph("endure-linux-dbg/gmail-testGmailComposeDiscard", "GmailComposeDiscard-l0-DMP", 0);</script> On 2013/02/01 06:29:50, Dai Mikurube wrote: > On 2013/02/01 01:33:35, dennis_jeffrey wrote: > > how did you decide upon this set of graphs to display on the overview page? > > I chose the number, four, since the overview.html should not have so many pages. > The reason why I picked them up is to have: > * both release and debug > * both pure-benchmarking and application (gmail) > > It's not so clear reason. Please tell me your suggestions if you have. > > > Also, I think about adding overview-endure.html which is similar to the page > generated by endure_result_parser.py. I see. Rather than adding a small subset of the Chrome Endure graphs to the main overview page, could we just add the whole overview-endure.html as a separate overview page that contains the full graphs? Maybe we can then just add a link to it from here (or maybe somewhere else if there's a better place to add the link?). I like that idea better than just showing 4 of the endurance graphs here.
Thanks, Mike and Dennis. I like the overview-endure.html idea. But, I think we should also have some graphs in overview.html. If only a link to overview-endure.html in overview.html, people (developers) don't get aware of the Endure graphs. DMP graphs are characteristic. In order to get more interest, I think we should have some graphs in overview.html. What do you think? https://codereview.chromium.org/12090088/diff/5001/dashboard/overview.html File dashboard/overview.html (right): https://codereview.chromium.org/12090088/diff/5001/dashboard/overview.html#ne... dashboard/overview.html:294: <script>DisplayGraph("endure-linux-rel/control-testControlAttachDetachDOMTree", "ControlAttachDetachDOMTree-TabMem-Private", 0);</script> On 2013/02/04 18:53:13, Mike Stipicevic wrote: > I noticed that none of the other calls to DisplayGraph include the graph > argument. Is it needed here, and has it been tested? It's tested locally. And, the main reason why we have the arguments is not to have "?history=50" in the links. The second reason is that the order of graphs in each graph page is unstable. I'm not sure what page will come on the top page.
On 2013/02/05 00:13:40, Dai Mikurube wrote: > Thanks, Mike and Dennis. > > I like the overview-endure.html idea. But, I think we should also have some > graphs in overview.html. If only a link to overview-endure.html in > overview.html, people (developers) don't get aware of the Endure graphs. > > DMP graphs are characteristic. In order to get more interest, I think we should > have some graphs in overview.html. What do you think? > Yes, that sounds reasonable. As long as the people looking at the overview page don't mind us adding a handful of new graphs to the page, then I think it's fine to add some endurance graphs that are representative. But it would also be great to have the overview-endure.html page available too, and easily-accessible. > https://codereview.chromium.org/12090088/diff/5001/dashboard/overview.html > File dashboard/overview.html (right): > > https://codereview.chromium.org/12090088/diff/5001/dashboard/overview.html#ne... > dashboard/overview.html:294: > <script>DisplayGraph("endure-linux-rel/control-testControlAttachDetachDOMTree", > "ControlAttachDetachDOMTree-TabMem-Private", 0);</script> > On 2013/02/04 18:53:13, Mike Stipicevic wrote: > > I noticed that none of the other calls to DisplayGraph include the graph > > argument. Is it needed here, and has it been tested? > > It's tested locally. And, the main reason why we have the arguments is not to > have "?history=50" in the links. The second reason is that the order of graphs > in each graph page is unstable. I'm not sure what page will come on the top > page.
On 2013/02/05 02:51:27, dennis_jeffrey wrote: > On 2013/02/05 00:13:40, Dai Mikurube wrote: > > Thanks, Mike and Dennis. > > > > I like the overview-endure.html idea. But, I think we should also have some > > graphs in overview.html. If only a link to overview-endure.html in > > overview.html, people (developers) don't get aware of the Endure graphs. > > > > DMP graphs are characteristic. In order to get more interest, I think we > should > > have some graphs in overview.html. What do you think? > > > > Yes, that sounds reasonable. As long as the people looking at the overview page > don't mind us adding a handful of new graphs to the page, then I think it's fine > to add some endurance graphs that are representative. But it would also be > great to have the overview-endure.html page available too, and > easily-accessible. > Agreed. I think it should be done in another CL (since a new file would require review in more detail. What do you think? > > https://codereview.chromium.org/12090088/diff/5001/dashboard/overview.html > > File dashboard/overview.html (right): > > > > > https://codereview.chromium.org/12090088/diff/5001/dashboard/overview.html#ne... > > dashboard/overview.html:294: > > > <script>DisplayGraph("endure-linux-rel/control-testControlAttachDetachDOMTree", > > "ControlAttachDetachDOMTree-TabMem-Private", 0);</script> > > On 2013/02/04 18:53:13, Mike Stipicevic wrote: > > > I noticed that none of the other calls to DisplayGraph include the graph > > > argument. Is it needed here, and has it been tested? > > > > It's tested locally. And, the main reason why we have the arguments is not to > > have "?history=50" in the links. The second reason is that the order of > graphs > > in each graph page is unstable. I'm not sure what page will come on the top > > page.
(fyi, I started working on overview-endure.html in another CL.)
LGTM
On 2013/02/06 00:41:33, dennis_jeffrey wrote: > LGTM Thanks. Committing...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmikurube@chromium.org/12090088/5001
Message was sent while issue was closed.
Change committed as 180842 |
