Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10871038:
Dump data for stacked graphs in Chrome Endure. (Closed)
|
Created:
8 years, 4 months ago by Dai Mikurube (NOT FULLTIME) Modified:
8 years, 3 months ago CC:
chromium-reviews, dennis_jeffrey, anantha, dyu1, Nirnimesh Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDump data for stacked graphs in Chrome Endure.
It also refactors Deep Memory Profiler in perf_endure.py. Code related to the profiler is extracted out to a class 'DeepMemoryProfiler'.
BUG=122119
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156238
Patch Set 1 #Patch Set 2 : ready #Patch Set 3 : use 'l2' #Patch Set 4 : refactored out Deep Memory Profiler #Patch Set 5 : renamed #
Total comments: 15
Patch Set 6 : fixed #Patch Set 7 : fixed docstring. #Patch Set 8 : rebased #Patch Set 9 : removed an empty line #
Total comments: 2
Patch Set 10 : fixed for Dennis's comment. #
Messages
Total messages: 13 (0 generated)
Hi Nirnimesh, It is a try for multiple performance values implemented in http://codereview.chromium.org/10870039/. For now, I uploaded this change just to share as an example. If you have any comments, please tell me.
Hi Dennis and Nirnimesh,
It is a change to dump a variation of -summary.dat for multi-line stacked
graphs. Could you take a look at this?
The JSON element names ('stacked' and 'default_stacking') may be changed later.
Hi, Updated the patch. The main difference is refactoring. ChromeEndureBaseTest got cleaner. Almost all the DMP-related code is now out of ChromeEndureBaseTest.
I really like how you put all of the DMP stuff inside a separate class to reduce coupling between the general Chrome Endure tests and DMP. http://codereview.chromium.org/10871038/diff/10001/chrome/test/functional/per... File chrome/test/functional/perf.py (right): http://codereview.chromium.org/10871038/diff/10001/chrome/test/functional/per... chrome/test/functional/perf.py:353: if 'default_stacking_order' not in new_line: should we change the name to something shorter like "stack_order"? We'll just assume that's the default and can be overridden elsewhere if needed. http://codereview.chromium.org/10871038/diff/10001/chrome/test/functional/per... chrome/test/functional/perf.py:374: def _OutputPerfGraphValue(self, description, value, units, Note: there are actually 2 ways in which perf results are outputted in this file: 1) By calling pyauto_utils.PrintPerfResult. This prints the results to stdout using a specially-formatted output line. These output lines are what are parsed in the live Chrome Endure bots running on the chromium.pyauto waterfall, and those results get written into text files that are displayed by the live chrome endure graphs. 2) By calling self._OutputPerfForStandaloneGraphing. These results are directly written to text data files to be displayed on local perf graphs only (not on the live Chrome Endure graphs). Currently, the changes in this file handle (2) but not (1). We'll need to also figure out how best to modify the format of the specially-formatted output lines to output information about graphs that should be stacked. Note also that the code that actually parses these stdout lines and creates the data files for the live Chrome Endure graphs is also not checked in yet, but that will have to be modified along with the changes here to get the stacked graphs to work on the live graphs. So, I will work on checking in that other code that parses the stdout of the live Chrome Endure bots to display on the live graphs. It's just a python script that runs every 2 hours on my local machine to update the data that's displayed on the live chrome endure graphs. http://codereview.chromium.org/10871038/diff/10001/chrome/test/functional/per... File chrome/test/functional/perf_endure.py (right): http://codereview.chromium.org/10871038/diff/10001/chrome/test/functional/per... chrome/test/functional/perf_endure.py:41: class DeepMemoryProfiler(object): add a docstring for this class http://codereview.chromium.org/10871038/diff/10001/chrome/test/functional/per... chrome/test/functional/perf_endure.py:60: if self._enabled: Rather than adding the "is enabled" checks at the start of all of these functions, maybe we should do the check at the call site instead (i.e., right before the function is invoked in the first place). I think that will enhance readability at the call sites because it makes it clear that those functions will only execute if DMP is enabled. http://codereview.chromium.org/10871038/diff/10001/chrome/test/functional/per... chrome/test/functional/perf_endure.py:147: remove this blank line; the comment above goes with the 2 lines below http://codereview.chromium.org/10871038/diff/10001/chrome/test/functional/per... chrome/test/functional/perf_endure.py:247: self._dmprof.RemoveTemporaryFiles() is there a reason this is done after calling the base class's tearDown()? If not, maybe we could move it up above to just below line 241, so the two function calls related to DMP are invoked one after the other.
Thank you for reviewing! Updated the patch. http://codereview.chromium.org/10871038/diff/10001/chrome/test/functional/per... File chrome/test/functional/perf.py (right): http://codereview.chromium.org/10871038/diff/10001/chrome/test/functional/per... chrome/test/functional/perf.py:353: if 'default_stacking_order' not in new_line: On 2012/09/05 17:34:12, dennis_jeffrey wrote: > should we change the name to something shorter like "stack_order"? We'll just > assume that's the default and can be overridden elsewhere if needed. Renamed: 'stacked' => 'stack' 'default_stacking_order' => 'stack_order' http://codereview.chromium.org/10871038/diff/10001/chrome/test/functional/per... chrome/test/functional/perf.py:374: def _OutputPerfGraphValue(self, description, value, units, On 2012/09/05 17:34:12, dennis_jeffrey wrote: > Note: there are actually 2 ways in which perf results are outputted in this > file: > > 1) By calling pyauto_utils.PrintPerfResult. This prints the results to stdout > using a specially-formatted output line. These output lines are what are parsed > in the live Chrome Endure bots running on the chromium.pyauto waterfall, and > those results get written into text files that are displayed by the live chrome > endure graphs. > > 2) By calling self._OutputPerfForStandaloneGraphing. These results are directly > written to text data files to be displayed on local perf graphs only (not on the > live Chrome Endure graphs). > > Currently, the changes in this file handle (2) but not (1). We'll need to also > figure out how best to modify the format of the specially-formatted output lines > to output information about graphs that should be stacked. Note also that the > code that actually parses these stdout lines and creates the data files for the > live Chrome Endure graphs is also not checked in yet, but that will have to be > modified along with the changes here to get the stacked graphs to work on the > live graphs. So, I will work on checking in that other code that parses the > stdout of the live Chrome Endure bots to display on the live graphs. It's just > a python script that runs every 2 hours on my local machine to update the data > that's displayed on the live chrome endure graphs. Thanks for your effort to commit the "RESULT" parsing code! I was wondering about that. For now, I added a TODO in _OutputPerfGraphValue. http://codereview.chromium.org/10871038/diff/10001/chrome/test/functional/per... File chrome/test/functional/perf_endure.py (right): http://codereview.chromium.org/10871038/diff/10001/chrome/test/functional/per... chrome/test/functional/perf_endure.py:41: class DeepMemoryProfiler(object): On 2012/09/05 17:34:12, dennis_jeffrey wrote: > add a docstring for this class Done. http://codereview.chromium.org/10871038/diff/10001/chrome/test/functional/per... chrome/test/functional/perf_endure.py:60: if self._enabled: On 2012/09/05 17:34:12, dennis_jeffrey wrote: > Rather than adding the "is enabled" checks at the start of all of these > functions, maybe we should do the check at the call site instead (i.e., right > before the function is invoked in the first place). I think that will enhance > readability at the call sites because it makes it clear that those functions > will only execute if DMP is enabled. Added checks at the call sites, but kept DeepMemoryProfiler-side as is for safety. http://codereview.chromium.org/10871038/diff/10001/chrome/test/functional/per... chrome/test/functional/perf_endure.py:147: On 2012/09/05 17:34:12, dennis_jeffrey wrote: > remove this blank line; the comment above goes with the 2 lines below Done. http://codereview.chromium.org/10871038/diff/10001/chrome/test/functional/per... chrome/test/functional/perf_endure.py:247: self._dmprof.RemoveTemporaryFiles() On 2012/09/05 17:34:12, dennis_jeffrey wrote: > is there a reason this is done after calling the base class's tearDown()? If > not, maybe we could move it up above to just below line 241, so the two function > calls related to DMP are invoked one after the other. It must be called after Chrome finishes. Chrome may dump last files at the end. Finally, merged the two functions into TearDown(). Now, it is called after perf.BasePerfTest.tearDown().
LGTM with 1 nit. http://codereview.chromium.org/10871038/diff/6006/chrome/test/functional/perf... File chrome/test/functional/perf_endure.py (right): http://codereview.chromium.org/10871038/diff/6006/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:172: maybe remove this blank line
Thanks for the comments. I'll be committing. http://codereview.chromium.org/10871038/diff/6006/chrome/test/functional/perf... File chrome/test/functional/perf_endure.py (right): http://codereview.chromium.org/10871038/diff/6006/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:172: On 2012/09/11 20:57:29, dennis_jeffrey wrote: > maybe remove this blank line Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmikurube@chromium.org/10871038/17001
Change committed as 156238
http://codereview.chromium.org/10871038/diff/10001/chrome/test/functional/per... File chrome/test/functional/perf.py (right): http://codereview.chromium.org/10871038/diff/10001/chrome/test/functional/per... chrome/test/functional/perf.py:374: def _OutputPerfGraphValue(self, description, value, units, Dennis and Dai, please let me know if/when there's something to review re: the change to the master-parsing code to handle the endure and also the stacked graphs. I'm interested to see how that code would change to support this (and of course to maintain backwards compatibility with the previous format). Thanks.
http://codereview.chromium.org/10871038/diff/10001/chrome/test/functional/per... File chrome/test/functional/perf.py (right): http://codereview.chromium.org/10871038/diff/10001/chrome/test/functional/per... chrome/test/functional/perf.py:374: def _OutputPerfGraphValue(self, description, value, units, On 2012/09/14 04:32:36, cmp (DO NOT USE) wrote: > Dennis and Dai, please let me know if/when there's something to review re: the > change to the master-parsing code to handle the endure and also the stacked > graphs. I'm interested to see how that code would change to support this (and > of course to maintain backwards compatibility with the previous format). > Thanks. Ah, ok, I'll Cc: you for such changes!
http://codereview.chromium.org/10871038/diff/10001/chrome/test/functional/per... File chrome/test/functional/perf.py (right): http://codereview.chromium.org/10871038/diff/10001/chrome/test/functional/per... chrome/test/functional/perf.py:374: def _OutputPerfGraphValue(self, description, value, units, On 2012/09/14 04:32:36, cmp (DO NOT USE) wrote: > Dennis and Dai, please let me know if/when there's something to review re: the > change to the master-parsing code to handle the endure and also the stacked > graphs. I'm interested to see how that code would change to support this (and > of course to maintain backwards compatibility with the previous format). > Thanks. Yes, actually I was planning to make you our main point of contact (and code reviewer) once we start modifying the master parsing code and the original chrome perf graphing code.
sgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
