Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9655016:
Enable Deep Memory Profiler in perf_endure.py. (Closed)
|
Created:
8 years, 9 months ago by Dai Mikurube (NOT FULLTIME) Modified:
8 years, 7 months ago CC:
chromium-reviews, dennis_jeffrey, John Grabowski, anantha, dyu1, Nirnimesh Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEnable Deep Memory Profiler in perf_endure.py.
It outputs 'tc-used-all' (Total bytes of allocated TCMalloc memory chunks) as the first step.
BUG=122119
TEST=run perf_endure.py with environment variables DEEP_MEMORY_PROFILE=True DEEP_MEMORY_PROFILE_INTERVAL=20
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=136248
Patch Set 1 #Patch Set 2 : rebased. #Patch Set 3 : updated. (rebased.) #Patch Set 4 : refined. #Patch Set 5 : #
Total comments: 35
Patch Set 6 : updated. (still ongoing work: contains testing/debugging code) #Patch Set 7 : refined. dmprof works in background. #
Total comments: 44
Patch Set 8 : Reflected Dennis's comments. #Patch Set 9 : modified comments. #Patch Set 10 : parse JSON. #Patch Set 11 : don't run DMP by default. #
Total comments: 4
Patch Set 12 : reflected the comments. #Patch Set 13 : a new function WaitForDeepMemoryProfiler #Patch Set 14 : modified and removed some TODOs. #Patch Set 15 : rebased. #Patch Set 16 : fixed for rebasing, and changed CHROME_BIN_PATH with BrowserPath() #Patch Set 17 : Output tc-used-all at first (example output) #
Total comments: 23
Patch Set 18 : reflected Dennis's comments. #Messages
Total messages: 28 (0 generated)
Hi Dennis, Nirnimesh, It's still ongoing, but could you take a brief look at this change? I wonder if you could tell me your opinions about a general outline of this change. http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... File chrome/test/functional/perf_endure.py (right): http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:145: if self._iteration_num % 20 == 5: Temporary trigger.
http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... File chrome/test/functional/perf_endure.py (right): http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:369: # TODO(dmikurube): Have to specify "--no-sandbox" for Chrome. Note: ExtraChromeFlags would work.
http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... File chrome/test/functional/perf_endure.py (right): http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:28: DMPROF_DIR_PATH = os.path.join(os.path.dirname(__file__), Unless you want these vars to be accessible from outside this file, prefix _ to them. Better stil, move them inside the class below. http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:45: 'chrome') What about non-linux? http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:145: if self._iteration_num % 20 == 5: On 2012/04/09 09:10:40, Dai Mikurube wrote: > Temporary trigger. Add comments / TODO notes. http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:253: print '^%s/endure.%05d.\d+.heap$' % ( Use the right logging call instead of print. http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:256: if re.match('^%s/endure.%05d.\d+.heap$' % ( use os.path.join() instead of hardcoding / http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:263: print 'First Dump: %s' % first_dump logging.info()? http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:267: with open('%s/endure.%05d.json' % (self._deep_tempdir, use os.path.join http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:269: p = subprocess.Popen( use better varname instead of |p| http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:275: p.wait() you won't need this if you use subprocess.call() in the previous line. http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:371: os.environ['HEAPPROFILE'] = '%s/endure' % self._deep_tempdir os.path.join
http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... File chrome/test/functional/perf_endure.py (right): http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:145: if self._iteration_num % 20 == 5: We'll likely want to be able to turn profiling on/off and specify the interval in which we dump profile data (or analyze the dumped data) http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:204: 'browser_pid': browser_proc_info['pid'], is this value used anywhere? http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:251: # TODO(dmikurube): Add --single in dmprof.py? So in general, a test will periodically call HeapProfilerDump(), and also periodically, we'll analyze the dumped data. What are the requirements for how often each of these things should occur? What are the tradeoffs involved in dumping more frequently, or analyzing the dumped data more frequently? http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:262: if first_dump: What does it mean for a file to be the "first dump", and why do we only need to use the first dump file? http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:368: def setUp(self): This only enables profiling in the two control tests, since it's inside of class ChromeEndureControlTest. Did you mean to enable profiling in all Chrome Endure tests?
Thank you for looking, Dennis and Nirnimesh. And, sorry for my less description. It's just on-going in tries-and-errors, so it contains many test printing, platform dependency, hard-coded values, temporary variables and so on... They'll be fixed eventually. Actually, I'd like your opinions about general outline like: - when and how often to dump, - is the directory adequate for temporary dumped files in Endure, - is my assumption about "working (current) directory" correct - is setting environment variables in setUp and tearDown is acceptable for the Endure, - ... since I don't know the Endure bot environment. (some of them were already answered, thanks! :)) http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... File chrome/test/functional/perf_endure.py (right): http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:145: if self._iteration_num % 20 == 5: On 2012/04/09 19:12:38, dennis_jeffrey wrote: > We'll likely want to be able to turn profiling on/off and specify the interval > in which we dump profile data (or analyze the dumped data) Ah, exactly. Such an option is required. How can I give an option to Endure from external? http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:251: # TODO(dmikurube): Add --single in dmprof.py? On 2012/04/09 19:12:38, dennis_jeffrey wrote: > So in general, a test will periodically call HeapProfilerDump(), and also > periodically, we'll analyze the dumped data. What are the requirements for how > often each of these things should occur? What are the tradeoffs involved in > dumping more frequently, or analyzing the dumped data more frequently? I'm in tries-and-errors about that. Dumping may take a little long stop-the-world (2-3 sec), and dumped data are large (3-4 MB or more for one dump). Frequent analysis has less negative impact. But, of course, more frequent analysis than dumping is just waste. (Yes, the current version does it.) http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:262: if first_dump: On 2012/04/09 19:12:38, dennis_jeffrey wrote: > What does it mean for a file to be the "first dump", and why do we only need to > use the first dump file? This is just a test printing, and will be removed eventually. The current deep memory profiler receives the first dump file, and finds next dump files by itself. Analyzing the same data many times is wasteful, but it's the easiest way to store the result in one file. (Re-analyzing doesn't cost so much.) I'm trying analyzing a single file, and storing its result into one file in dmprof.py. http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:269: p = subprocess.Popen( On 2012/04/09 18:46:44, Nirnimesh wrote: > use better varname instead of |p| Good catch. Thanks. http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:368: def setUp(self): On 2012/04/09 19:12:38, dennis_jeffrey wrote: > This only enables profiling in the two control tests, since it's inside of class > ChromeEndureControlTest. Did you mean to enable profiling in all Chrome Endure > tests? Finally, yes. Is there an efficient way to enable it for all Endure tests?
I answered a few questions you had in your comments, and here are some additional thoughts about some issues that you brought up: 1) when and how often to dump I'm not sure. Suppose we turn on profiling, let the test run for the full 6 hours, then dump everything only once at the very end of the test to be analyzed. What would the problems with that approach be? Would it be too much data to store in memory? Alternatively, what if we dump/analyze at the same time that the Chrome Endure tests already collect perf data (once every 10 minutes). Would that be reasonable? 2) is the directory adequate for temporary dumped files in Endure Right now you use a temp directory, and I think that's fine. We'll just want to make sure the test cleans up the created temporary dump files. 3) is my assumption about "working (current) directory" correct Are you referring to how you locate dmprof.py and chrome using relative paths? I think that's also ok. 4) is setting environment variables in setUp and tearDown is acceptable for the Endure Yes, in fact Chrome Endure already has code in the base setUp function to look for a couple environment variables. That's currently how we pass inputs to only the Chrome Endure tests. http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... File chrome/test/functional/perf_endure.py (right): http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:145: if self._iteration_num % 20 == 5: On 2012/04/11 08:44:02, Dai Mikurube wrote: > On 2012/04/09 19:12:38, dennis_jeffrey wrote: > > We'll likely want to be able to turn profiling on/off and specify the interval > > in which we dump profile data (or analyze the dumped data) > > Ah, exactly. Such an option is required. How can I give an option to Endure > from external? Right now it's done through environment variables. See lines 60-63 above. http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:368: def setUp(self): On 2012/04/11 08:44:02, Dai Mikurube wrote: > On 2012/04/09 19:12:38, dennis_jeffrey wrote: > > This only enables profiling in the two control tests, since it's inside of > class > > ChromeEndureControlTest. Did you mean to enable profiling in all Chrome > Endure > > tests? > > Finally, yes. Is there an efficient way to enable it for all Endure tests? Yes - if you enable profiling above in class ChromeEndureBaseTest (see setUp function in line 57), then it will be enabled for all Chrome Endure tests, since all Chrome Endure tests execute that setUp function.
Thanks, Dennis. At first, I was forgetting an important fact about PyAuto API. When I call HeapProfilerDump() from a PyAuto script, only browser process's information will be dumped since testing_automation_provider is working in the browser process. I think I'll use an env HEAP_PROFILE_TIME_INTERVAL to dump (with this, DMP dumps repeatedly by itself independently from perf_endure.py) for now. I wonder if you could tell me an example of IPC to a renderer process in PyAuto API, if you know. What I'd like to do is a PyAuto API function, for example, "void HeapProfilerDump(int child_pid)" or something like that. On 2012/04/12 00:51:52, dennis_jeffrey wrote: > I answered a few questions you had in your comments, and here are some > additional thoughts about some issues that you brought up: > > 1) when and how often to dump > > I'm not sure. Suppose we turn on profiling, let the test run for the full 6 > hours, then dump everything only once at the very end of the test to be > analyzed. What would the problems with that approach be? Would it be too much > data to store in memory? > > Alternatively, what if we dump/analyze at the same time that the Chrome Endure > tests already collect perf data (once every 10 minutes). Would that be > reasonable? > Taking snapshots on memory will consume so much memory, so the former sounds like impractical. I guess Dumping at the same time with GetPerfStats is enough reasonable, if it is acceptable for Endure that DMP stops the world for 1-2 seconds every 10 minutes. > 2) is the directory adequate for temporary dumped files in Endure > > Right now you use a temp directory, and I think that's fine. We'll just want to > make sure the test cleans up the created temporary dump files. > Ok. Cleaning-up is commented out in the uploaded code, but it's temporary. > 3) is my assumption about "working (current) directory" correct > > Are you referring to how you locate dmprof.py and chrome using relative paths? > I think that's also ok. > Thanks. > 4) is setting environment variables in setUp and tearDown is acceptable for the > Endure > > Yes, in fact Chrome Endure already has code in the base setUp function to look > for a couple environment variables. That's currently how we pass inputs to only > the Chrome Endure tests. > I see. Thank you. http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... File chrome/test/functional/perf_endure.py (right): http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:28: DMPROF_DIR_PATH = os.path.join(os.path.dirname(__file__), On 2012/04/09 18:46:44, Nirnimesh wrote: > Unless you want these vars to be accessible from outside this file, prefix _ to > them. Better stil, move them inside the class below. Done. http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:45: 'chrome') On 2012/04/09 18:46:44, Nirnimesh wrote: > What about non-linux? Done. http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:145: if self._iteration_num % 20 == 5: On 2012/04/12 00:51:52, dennis_jeffrey wrote: > On 2012/04/11 08:44:02, Dai Mikurube wrote: > > On 2012/04/09 19:12:38, dennis_jeffrey wrote: > > > We'll likely want to be able to turn profiling on/off and specify the > interval > > > in which we dump profile data (or analyze the dumped data) > > > > Ah, exactly. Such an option is required. How can I give an option to Endure > > from external? > > Right now it's done through environment variables. See lines 60-63 above. Using an environment variable DEEP_MEMORY_PROFILE_INTERVAL. Thanks. http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:204: 'browser_pid': browser_proc_info['pid'], On 2012/04/09 19:12:38, dennis_jeffrey wrote: > is this value used anywhere? No for now. Removed it. http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:267: with open('%s/endure.%05d.json' % (self._deep_tempdir, On 2012/04/09 18:46:44, Nirnimesh wrote: > use os.path.join Done. http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:368: def setUp(self): On 2012/04/12 00:51:52, dennis_jeffrey wrote: > On 2012/04/11 08:44:02, Dai Mikurube wrote: > > On 2012/04/09 19:12:38, dennis_jeffrey wrote: > > > This only enables profiling in the two control tests, since it's inside of > > class > > > ChromeEndureControlTest. Did you mean to enable profiling in all Chrome > > Endure > > > tests? > > > > Finally, yes. Is there an efficient way to enable it for all Endure tests? > > Yes - if you enable profiling above in class ChromeEndureBaseTest (see setUp > function in line 57), then it will be enabled for all Chrome Endure tests, since > all Chrome Endure tests execute that setUp function. I see. Thank you. I did it in the uploaded code. http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:371: os.environ['HEAPPROFILE'] = '%s/endure' % self._deep_tempdir On 2012/04/09 18:46:44, Nirnimesh wrote: > os.path.join Done.
Updated. This version supports background dmprof execution since dmprof may take long time (especially for the first run). Besides above, this version uses HEAP_PROFILE_TIME_INTERVAL to dump data instead of calling HeapProfilerDump() PyAuto API. HeapProfilerDump() for renderer processes is to be done (as I wrote in the previous comment). http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... File chrome/test/functional/perf_endure.py (right): http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:253: print '^%s/endure.%05d.\d+.heap$' % ( On 2012/04/09 18:46:44, Nirnimesh wrote: > Use the right logging call instead of print. Done. http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:256: if re.match('^%s/endure.%05d.\d+.heap$' % ( On 2012/04/09 18:46:44, Nirnimesh wrote: > use os.path.join() instead of hardcoding / Done. http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:263: print 'First Dump: %s' % first_dump On 2012/04/09 18:46:44, Nirnimesh wrote: > logging.info()? Done. http://codereview.chromium.org/9655016/diff/10001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:275: p.wait() On 2012/04/09 18:46:44, Nirnimesh wrote: > you won't need this if you use subprocess.call() in the previous line. Finally, this script is required to run on background. (dmprof may take long time.) The latest version supports background execution with Popen. Please look at that.
Hi Dai - I don't believe that pyauto can interact directly with a renderer process. Pyauto can make calls into chrome's automation module though, which can in turn interact with a renderer process. So I think you'd have to modify things on the automation hook side in order to get pyauto to tell a dump to occur from a renderer process. http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... File chrome/test/functional/perf_endure.py (right): http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:12: DEEP_MEMORY_PROFILE_INTERVAL: The number of seconds to wait in-between each nit: add a blank line above this to separate the Deep Memory Profiler variables from the other variables. http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:12: DEEP_MEMORY_PROFILE_INTERVAL: The number of seconds to wait in-between each Right now, this variable seems to control whether or not the Deep Memory Profiler is used. By default, it seems to be turned on. Do you think we should use another variable, "DEEP_HEAP_PROFILE", to instead decide whether or not to use the Deep Profiler? And maybe we should set it to False by default (it requires a DEBUG build, right?). http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:50: 'deep_memory_profiler') I recommend fitting as many parameters as you can on a line: _DMPROF_DIR_PATH = os.path.join( os.path.dirname(__file__), os.pardir, os.pardir, os.pardir, 'tools', 'deep_memory_profiler') http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:53: 'dmprof') this can probably fit on the previous line http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:62: 'chrome') same comment as line 50 above http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:79: # TODO(dmikurube): Remove it when renderer dump is supported in PyAuto remove what - the following environment variable definition? http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:119: self._deep_memory_profile_json = None The variable name here suggests that it will store a JSON string, but it instead stores a file object. Maybe we should make that more clear in the variable name http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:127: # TODO(dmikurube): Remove it when renderer dump is supported in PyAuto remove what? the next line? the next 4 lines? http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:134: shutil.rmtree(self._deep_tempdir) we can invoke pyauto_utils.RemovePath() here instead (import pyauto_utils at the top of the file, rather than shutil). http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:134: shutil.rmtree(self._deep_tempdir) be careful, I don't think we can assume that self._deep_tempdir would have necessarily been defined once we get here. http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:146: self._DEEP_MEMORY_PROFILE_INTERVAL)) isn't this work already done in setUp() above? http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:155: ['--remote-debugging-port=9222']) extra_flags = ['--remote-debugging-port=9222'] if deep_memory_profile_interval > 0: extra_flags.append('--no-sandbox') return perf.BasePerfTest.ExtraChromeFlags(self) + extra_flags http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:191: # Note: this block will be removed when committing. if this block will get removed before committing, remember also to delete the variable defined at line 179 above http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:217: webapp_name, test_description, tab_title_substring, True) is_last=True http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:284: the tab to use, for identifying the appropriate tab. describe the new is_last argument http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:316: if is_last and self._deep_memory_profile_proc: should this check go inside the condition at line 325 below? It seems that we don't need to do this check unless we're using the Deep Memory Profiler in the first place http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:329: logging.info(' Last dmprof is still running.') Is it ok to just skip the dump analysis on this invocation of _GetPerformanceStats if the previous invocation of dmprof is still running? Would it be better to wait for the previous invocation of dmprof to finish instead? http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:368: # 2) Parse .json (including only the latest snapshot) file here. shouldn't this parsing work be inside the "if" at line 325 above? We should only do it if we're using the Deep Memory Profiler http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:391: # 3) Do self._OutputPerfGraphValue for multiple lines, here. What multiple lines need to be outputted here? Should this be a TODO?
Thanks, Dennis. Ok, so I should add an IPC interface on the renderer side, and call the IPC function from testing_automation_provider... I'll try it later. Now, this change is basically ready to output graphs. When an interface to output multi-lined graphs is ready, it's time to add DMP graphs into Endure. :) (After code reviews, of course) http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... File chrome/test/functional/perf_endure.py (right): http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:12: DEEP_MEMORY_PROFILE_INTERVAL: The number of seconds to wait in-between each On 2012/04/17 01:20:27, dennis_jeffrey wrote: > nit: add a blank line above this to separate the Deep Memory Profiler variables > from the other variables. Done. http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:12: DEEP_MEMORY_PROFILE_INTERVAL: The number of seconds to wait in-between each On 2012/04/17 01:20:27, dennis_jeffrey wrote: > Right now, this variable seems to control whether or not the Deep Memory > Profiler is used. By default, it seems to be turned on. Do you think we should > use another variable, "DEEP_HEAP_PROFILE", to instead decide whether or not to > use the Deep Profiler? And maybe we should set it to False by default (it > requires a DEBUG build, right?). I thought about such a flag to turn on/off, but I stopped the choice since too many envs don't sound good. Deep Memory Profiler is disabled when it's zero. How about to set the default to zero? http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:50: 'deep_memory_profiler') On 2012/04/17 01:20:27, dennis_jeffrey wrote: > I recommend fitting as many parameters as you can on a line: > > _DMPROF_DIR_PATH = os.path.join( > os.path.dirname(__file__), os.pardir, os.pardir, os.pardir, > 'tools', 'deep_memory_profiler') Done. http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:53: 'dmprof') On 2012/04/17 01:20:27, dennis_jeffrey wrote: > this can probably fit on the previous line Done. http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:62: 'chrome') On 2012/04/17 01:20:27, dennis_jeffrey wrote: > same comment as line 50 above Done. http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:79: # TODO(dmikurube): Remove it when renderer dump is supported in PyAuto On 2012/04/17 01:20:27, dennis_jeffrey wrote: > remove what - the following environment variable definition? Changed it to: Stop to set HEAP_PROFILE_TIME_INTERVAL when PyAuto supports to dump renderer heap profile. http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:119: self._deep_memory_profile_json = None On 2012/04/17 01:20:27, dennis_jeffrey wrote: > The variable name here suggests that it will store a JSON string, but it instead > stores a file object. Maybe we should make that more clear in the variable name Added '_f'. http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:127: # TODO(dmikurube): Remove it when renderer dump is supported in PyAuto On 2012/04/17 01:20:27, dennis_jeffrey wrote: > remove what? the next line? the next 4 lines? Changed in the same way above. http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:134: shutil.rmtree(self._deep_tempdir) On 2012/04/17 01:20:27, dennis_jeffrey wrote: > be careful, I don't think we can assume that self._deep_tempdir would have > necessarily been defined once we get here. Added conditions. http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:134: shutil.rmtree(self._deep_tempdir) On 2012/04/17 01:20:27, dennis_jeffrey wrote: > we can invoke pyauto_utils.RemovePath() here instead (import pyauto_utils at the > top of the file, rather than shutil). Done... but top of the file? http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:146: self._DEEP_MEMORY_PROFILE_INTERVAL)) On 2012/04/17 01:20:27, dennis_jeffrey wrote: > isn't this work already done in setUp() above? ExtraChromeFlags looks like called before setUp(). http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:155: ['--remote-debugging-port=9222']) On 2012/04/17 01:20:27, dennis_jeffrey wrote: > extra_flags = ['--remote-debugging-port=9222'] > if deep_memory_profile_interval > 0: > extra_flags.append('--no-sandbox') > return perf.BasePerfTest.ExtraChromeFlags(self) + extra_flags Done. http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:191: # Note: this block will be removed when committing. On 2012/04/17 01:20:27, dennis_jeffrey wrote: > if this block will get removed before committing, remember also to delete the > variable defined at line 179 above Ok, added a commment. http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:217: webapp_name, test_description, tab_title_substring, True) On 2012/04/17 01:20:27, dennis_jeffrey wrote: > is_last=True Done. http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:284: the tab to use, for identifying the appropriate tab. On 2012/04/17 01:20:27, dennis_jeffrey wrote: > describe the new is_last argument Done. http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:316: if is_last and self._deep_memory_profile_proc: On 2012/04/17 01:20:27, dennis_jeffrey wrote: > should this check go inside the condition at line 325 below? It seems that we > don't need to do this check unless we're using the Deep Memory Profiler in the > first place Exactly. Moved this block into : "if self._deep_memory_profile_interval > 0:" And, added a comment. If we don't check it, the last _GetPerformanceStats call may miss last dump files when a previous dmprof process is still running. http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:329: logging.info(' Last dmprof is still running.') On 2012/04/17 01:20:27, dennis_jeffrey wrote: > Is it ok to just skip the dump analysis on this invocation of > _GetPerformanceStats if the previous invocation of dmprof is still running? > Would it be better to wait for the previous invocation of dmprof to finish > instead? Actually, a single dmprof run for 'first_dump' will profile all continuous dump files. In an extreme case, we can execute dmprof only at last to generate a graph eventually. The reason why I call dmprof in the loop is to generate a graph in semi-real time. The current dmprof may take long time...for 3-4 minutes or more. It doesn't matter in usual Endure cases (every 10 mins), but, it's a problem for more frequent cases: it stops _GetPerformanceStats for 3-4 mins. btw, the reason of dmprof's speed is calling 'pprof --symbols' to get symbol information from a Chrome binary. I'm planning to replace it for speed-up. http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:368: # 2) Parse .json (including only the latest snapshot) file here. On 2012/04/17 01:20:27, dennis_jeffrey wrote: > shouldn't this parsing work be inside the "if" at line 325 above? We should > only do it if we're using the Deep Memory Profiler Exactly. Moved the comment. http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:391: # 3) Do self._OutputPerfGraphValue for multiple lines, here. On 2012/04/17 01:20:27, dennis_jeffrey wrote: > What multiple lines need to be outputted here? Should this be a TODO? Maybe a TODO. I'd like to output data such as: { 'webcore-dom': [7210945, 1374271, ...], 'webcore-fontcache': [21938176, 22740992, ...], ... # 'component-name': [array of memory sizes in time order] }
Thanks! Also, I was talking to gpike and he mentioned again about the issue with address space randomization on ChromeOS. He also thinks that for Chrome linux, address space randomization is turned on by default, and that may affect profiling results. Is that something that we need to consider when running the Deep Memory Profiler on Chrome Endure, since these tests currently run on Chrome linux? http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... File chrome/test/functional/perf_endure.py (right): http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:12: DEEP_MEMORY_PROFILE_INTERVAL: The number of seconds to wait in-between each On 2012/04/17 06:33:11, Dai Mikurube wrote: > On 2012/04/17 01:20:27, dennis_jeffrey wrote: > > Right now, this variable seems to control whether or not the Deep Memory > > Profiler is used. By default, it seems to be turned on. Do you think we > should > > use another variable, "DEEP_HEAP_PROFILE", to instead decide whether or not to > > use the Deep Profiler? And maybe we should set it to False by default (it > > requires a DEBUG build, right?). > > I thought about such a flag to turn on/off, but I stopped the choice since too > many envs don't sound good. > > Deep Memory Profiler is disabled when it's zero. How about to set the default > to zero? I thought about setting the default to 0, but then it's more difficult if we want to: (1) turn on the Deep Profiler, and (2) want to use the same interval as PER_STATS_INTERVAL. Maybe it's ok to add a new env variable named "DEEP_HEAP_PROFILE", since anyway that's a variable that would need to be set if using the Deep Profiler, right? We can just move the definition from within this test, to outside the test - if the user of Chrome Endure wants to turn on the Deep Profiler. What do you think? http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:146: self._DEEP_MEMORY_PROFILE_INTERVAL)) On 2012/04/17 06:33:11, Dai Mikurube wrote: > On 2012/04/17 01:20:27, dennis_jeffrey wrote: > > isn't this work already done in setUp() above? > > ExtraChromeFlags looks like called before setUp(). Ok - then maybe it's best to write a little helper function to look up this environment variable? That way we don't have to duplicate the code in both places. Or, maybe we should read the environment variable here only, and not in setUp? http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:329: logging.info(' Last dmprof is still running.') On 2012/04/17 06:33:11, Dai Mikurube wrote: > On 2012/04/17 01:20:27, dennis_jeffrey wrote: > > Is it ok to just skip the dump analysis on this invocation of > > _GetPerformanceStats if the previous invocation of dmprof is still running? > > Would it be better to wait for the previous invocation of dmprof to finish > > instead? > > Actually, a single dmprof run for 'first_dump' will profile all continuous dump > files. In an extreme case, we can execute dmprof only at last to generate a > graph eventually. > > The reason why I call dmprof in the loop is to generate a graph in semi-real > time. The current dmprof may take long time...for 3-4 minutes or more. It > doesn't matter in usual Endure cases (every 10 mins), but, it's a problem for > more frequent cases: it stops _GetPerformanceStats for 3-4 mins. > > btw, the reason of dmprof's speed is calling 'pprof --symbols' to get symbol > information from a Chrome binary. I'm planning to replace it for speed-up. Ok. This shouldn't have any impact on the memory usage of Chrome or its renderer processes (i.e., the results of these perf tests), right? http://codereview.chromium.org/9655016/diff/26001/chrome/test/functional/perf... File chrome/test/functional/perf_endure.py (right): http://codereview.chromium.org/9655016/diff/26001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:115: self._deep_memory_profile_json_f = None I recommend using "file" instead of just "f", just to make it clear http://codereview.chromium.org/9655016/diff/26001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:129: perf.BasePerfTest.tearDown(self) # Must be done at end of this function. Maybe we should move this line to the bottom of the current function. That will be consistent with what the comment suggests ;-)
Thanks, Dennis. For address space randomization, DMP is showing reasonable results in Chrome Linux. No randomized results, e.g. V8 heap reasonably grows, DOM objects increase/decrease as operations; it means that symbols for V8 and WebKit function are correctly resolved from memory addresses. I don't specify any special compiling options. I only give "--no-sandbox" for Chrome/Debug-build at runtime... http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... File chrome/test/functional/perf_endure.py (right): http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:12: DEEP_MEMORY_PROFILE_INTERVAL: The number of seconds to wait in-between each On 2012/04/17 22:36:14, dennis_jeffrey wrote: > On 2012/04/17 06:33:11, Dai Mikurube wrote: > > On 2012/04/17 01:20:27, dennis_jeffrey wrote: > > > Right now, this variable seems to control whether or not the Deep Memory > > > Profiler is used. By default, it seems to be turned on. Do you think we > > should > > > use another variable, "DEEP_HEAP_PROFILE", to instead decide whether or not > to > > > use the Deep Profiler? And maybe we should set it to False by default (it > > > requires a DEBUG build, right?). > > > > I thought about such a flag to turn on/off, but I stopped the choice since too > > many envs don't sound good. > > > > Deep Memory Profiler is disabled when it's zero. How about to set the default > > to zero? > > I thought about setting the default to 0, but then it's more difficult if we > want to: (1) turn on the Deep Profiler, and (2) want to use the same interval as > PER_STATS_INTERVAL. > > Maybe it's ok to add a new env variable named "DEEP_HEAP_PROFILE", since anyway > that's a variable that would need to be set if using the Deep Profiler, right? > We can just move the definition from within this test, to outside the test - if > the user of Chrome Endure wants to turn on the Deep Profiler. What do you > think? Basically agreed. Added an env "DEEP_MEMORY_PROFILE". But actually, I didn't understand "We can just move the definition from within this test, to outside the test"... you mean moving '_DEEP_MEMORY_PROFILE' out of ChromeEndureBaseTest? http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:146: self._DEEP_MEMORY_PROFILE_INTERVAL)) On 2012/04/17 22:36:14, dennis_jeffrey wrote: > On 2012/04/17 06:33:11, Dai Mikurube wrote: > > On 2012/04/17 01:20:27, dennis_jeffrey wrote: > > > isn't this work already done in setUp() above? > > > > ExtraChromeFlags looks like called before setUp(). > > Ok - then maybe it's best to write a little helper function to look up this > environment variable? That way we don't have to duplicate the code in both > places. Or, maybe we should read the environment variable here only, and not in > setUp? Since I'm not sure that ExtraChromeFlags is always called before setUp in future, I introduces a new helper function 'GetDeepMemoryProfileEnv'. http://codereview.chromium.org/9655016/diff/20001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:329: logging.info(' Last dmprof is still running.') On 2012/04/17 22:36:14, dennis_jeffrey wrote: > On 2012/04/17 06:33:11, Dai Mikurube wrote: > > On 2012/04/17 01:20:27, dennis_jeffrey wrote: > > > Is it ok to just skip the dump analysis on this invocation of > > > _GetPerformanceStats if the previous invocation of dmprof is still running? > > > Would it be better to wait for the previous invocation of dmprof to finish > > > instead? > > > > Actually, a single dmprof run for 'first_dump' will profile all continuous > dump > > files. In an extreme case, we can execute dmprof only at last to generate a > > graph eventually. > > > > The reason why I call dmprof in the loop is to generate a graph in semi-real > > time. The current dmprof may take long time...for 3-4 minutes or more. It > > doesn't matter in usual Endure cases (every 10 mins), but, it's a problem for > > more frequent cases: it stops _GetPerformanceStats for 3-4 mins. > > > > btw, the reason of dmprof's speed is calling 'pprof --symbols' to get symbol > > information from a Chrome binary. I'm planning to replace it for speed-up. > > Ok. This shouldn't have any impact on the memory usage of Chrome or its > renderer processes (i.e., the results of these perf tests), right? No impact. Profile data have already been dumped to files by HEAP_PROFILE_TIME_INTERVAL. It just runs an analyzing script for the dumped profile data. http://codereview.chromium.org/9655016/diff/26001/chrome/test/functional/perf... File chrome/test/functional/perf_endure.py (right): http://codereview.chromium.org/9655016/diff/26001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:115: self._deep_memory_profile_json_f = None On 2012/04/17 22:36:14, dennis_jeffrey wrote: > I recommend using "file" instead of just "f", just to make it clear Done. http://codereview.chromium.org/9655016/diff/26001/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:129: perf.BasePerfTest.tearDown(self) # Must be done at end of this function. On 2012/04/17 22:36:14, dennis_jeffrey wrote: > Maybe we should move this line to the bottom of the current function. That will > be consistent with what the comment suggests ;-) We have to remove the temporary directory after Chrome finishes, so I modified the comments.
Besides, please ping me when multi-line graphs are available for perf_endure. :)
Hi Dai - I'll let you know about the multi-line graphs. I'm trying to get some work done on it this week. Do you think it might be helpful to talk to gpike@ about the possibility of address space randomization? I know for sure he pointed out this issue would need to be resolved for DMP to work on ChromeOS. And if it's not an issue on Chrome linux, it would be good to understand why. What do you think? I know very little about it myself. -Dennis On Wed, Apr 18, 2012 at 12:44 AM, <dmikurube@chromium.org> wrote: > Besides, please ping me when multi-line graphs are available for > perf_endure. :) > > http://codereview.chromium.**org/9655016/<http://codereview.chromium.org/9655... >
Thank you, Dennis. Talking w/ gpike@ sounds helpful to understand the current situation. E-mails might work, but I can block a time-slot to talk via VC. (Timezone difference is a problem, though...) btw, does the latest _OutputPerfGraphValue accept only a new value, not an array of all values? On 2012/04/19 00:45:02, dennisjeffrey wrote: > Hi Dai - I'll let you know about the multi-line graphs. I'm trying to get > some work done on it this week. > > Do you think it might be helpful to talk to gpike@ about the possibility of > address space randomization? I know for sure he pointed out this issue > would need to be resolved for DMP to work on ChromeOS. And if it's not an > issue on Chrome linux, it would be good to understand why. What do you > think? I know very little about it myself. > > -Dennis > > On Wed, Apr 18, 2012 at 12:44 AM, <mailto:dmikurube@chromium.org> wrote: > > > Besides, please ping me when multi-line graphs are available for > > perf_endure. :) > > > > > http://codereview.chromium.**org/9655016/%3Chttp://codereview.chromium.org/96...> > >
Hi Dai, The latest _OutputPerfGraphValue still accepts an array of all values like before. However, the recent change in this CL<https://chromiumcodereview.appspot.com/10037017/>affects how the list is interpreted for graphing purposes, and this change requires users of the function to also change their behavior. In the old way of how things worked, a test would build up a list of values over time during a test, and that list would always keep getting larger. Whenever you periodically called _OutputPerfGraphValue, you'd pass the whole list, which would cause the old values that were already outputted before, to be outputted again, along with the newer values. As a result, the scripts that parsed the outputted perf measurements always kept only the last outputted value, since that included the full data. The change in the recent CL mentioned above is designed to remove the redundancy in the output of the test. Now, whenever you call _OutputPerfGraphValue, you should output only the *new* perf measurements that were taken since the last time you called _OutputPerfGraphValue (even if there's only 1 new value since the last time you outputted perf values, you'd output a list containing just that 1 new element). -Dennis On Wed, Apr 18, 2012 at 7:33 PM, <dmikurube@chromium.org> wrote: > Thank you, Dennis. > > Talking w/ gpike@ sounds helpful to understand the current situation. > E-mails > might work, but I can block a time-slot to talk via VC. (Timezone > difference is > a problem, though...) > > btw, does the latest _OutputPerfGraphValue accept only a new value, not an > array > of all values? > > > On 2012/04/19 00:45:02, dennisjeffrey wrote: > >> Hi Dai - I'll let you know about the multi-line graphs. I'm trying to get >> some work done on it this week. >> > > Do you think it might be helpful to talk to gpike@ about the possibility >> of >> address space randomization? I know for sure he pointed out this issue >> would need to be resolved for DMP to work on ChromeOS. And if it's not an >> issue on Chrome linux, it would be good to understand why. What do you >> think? I know very little about it myself. >> > > -Dennis >> > > On Wed, Apr 18, 2012 at 12:44 AM, <mailto:dmikurube@chromium.org**> >> wrote: >> > > > Besides, please ping me when multi-line graphs are available for >> > perf_endure. :) >> > >> > >> > > http://codereview.chromium.****org/9655016/%3Chttp://coderevi** > ew.chromium.org/9655016/ <http://codereview.chromium.org/9655016/>> > >> > >> > > > http://codereview.chromium.**org/9655016/<http://codereview.chromium.org/9655... >
Hi Dennis, Thank you for the description. Ok - I'll add a code to compute difference from the previous _OutputPerfGraphValue when adding graph drawing. It may be a little complicated (since dmprof runs in background), but not a problem. :) On 2012/04/20 00:50:01, dennisjeffrey wrote: > Hi Dai, > > The latest _OutputPerfGraphValue still accepts an array of all values like > before. However, the recent change in this > CL<https://chromiumcodereview.appspot.com/10037017/>affects how the > list is interpreted for graphing purposes, and this change > requires users of the function to also change their behavior. > > In the old way of how things worked, a test would build up a list of values > over time during a test, and that list would always keep getting larger. > Whenever you periodically called _OutputPerfGraphValue, you'd pass the > whole list, which would cause the old values that were already outputted > before, to be outputted again, along with the newer values. As a result, > the scripts that parsed the outputted perf measurements always kept only > the last outputted value, since that included the full data. > > The change in the recent CL mentioned above is designed to remove the > redundancy in the output of the test. Now, whenever you call > _OutputPerfGraphValue, you should output only the *new* perf measurements > that were taken since the last time you called _OutputPerfGraphValue (even > if there's only 1 new value since the last time you outputted perf values, > you'd output a list containing just that 1 new element). > > -Dennis
Hi Dennis, Nirnimesh, Updated the patch. As the first step, it outputs 'tc-used-all' values by DMP which represent total bytes of allocated TCMalloc memory chunks. I wonder if I could commit this change as the first step. Could you take a look at it?
Cool! Some comments for the latest patch set. http://codereview.chromium.org/9655016/diff/42002/chrome/test/functional/perf... File chrome/test/functional/perf_endure.py (right): http://codereview.chromium.org/9655016/diff/42002/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:35: add 1 more blank line here http://codereview.chromium.org/9655016/diff/42002/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:55: _CHROME_BIN_PATH = os.path.join(perf.BasePerfTest.BrowserPath(), 'chrome') I think we can just say "self.BrowserPath()" here http://codereview.chromium.org/9655016/diff/42002/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:62: print self._deep_memory_profile i recommend using logging.debug or logging.info, or else remove this line http://codereview.chromium.org/9655016/diff/42002/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:77: '%d' % self._deep_memory_profile_interval) os.environ['HEAP_PROFILE_TIME_INTERVAL'] = ( str(self._deep_memory_profile_interval)) http://codereview.chromium.org/9655016/diff/42002/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:109: self._iteration_num = 0 i think this line is unnecessary http://codereview.chromium.org/9655016/diff/42002/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:138: def GetDeepMemoryProfileEnv( prefix the function name with an underscore if it's meant to be used only locally to this function. http://codereview.chromium.org/9655016/diff/42002/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:151: result = not_supported rather than having a "not_supported" value, should we just raise an exception if the current platform is not supported? http://codereview.chromium.org/9655016/diff/42002/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:156: def WaitForDeepMemoryProfiler(self): prefix the function name with an underscore if it's meant to be used only locally to this function. http://codereview.chromium.org/9655016/diff/42002/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:157: """Waits for the Deep Memory Profiler finished if running. 'finished' --> 'to finish' http://codereview.chromium.org/9655016/diff/42002/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:158: """ move this to the previous line if it fits there http://codereview.chromium.org/9655016/diff/42002/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:444: for index in range(self._deep_memory_profile_last_index, rather than looping through each index and separately calling _OutputPerfGraphValue for each one, we could build up a list of tuples and then output the whole list of tuples in a single call to _OutputPerfGraphValue.
Thank you, Dennis! Updated the patch. http://codereview.chromium.org/9655016/diff/42002/chrome/test/functional/perf... File chrome/test/functional/perf_endure.py (right): http://codereview.chromium.org/9655016/diff/42002/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:35: On 2012/05/09 04:31:47, dennis_jeffrey wrote: > add 1 more blank line here Done. http://codereview.chromium.org/9655016/diff/42002/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:55: _CHROME_BIN_PATH = os.path.join(perf.BasePerfTest.BrowserPath(), 'chrome') On 2012/05/09 04:31:47, dennis_jeffrey wrote: > I think we can just say "self.BrowserPath()" here Since it's not in "def something(self)", we can't use self here. I couldn't use just "BrowserPath()" and "BasePerfTest.BrowserPath()", too.. http://codereview.chromium.org/9655016/diff/42002/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:62: print self._deep_memory_profile On 2012/05/09 04:31:47, dennis_jeffrey wrote: > i recommend using logging.debug or logging.info, or else remove this line Thanks. Removed this line. http://codereview.chromium.org/9655016/diff/42002/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:77: '%d' % self._deep_memory_profile_interval) On 2012/05/09 04:31:47, dennis_jeffrey wrote: > os.environ['HEAP_PROFILE_TIME_INTERVAL'] = ( > str(self._deep_memory_profile_interval)) Done. http://codereview.chromium.org/9655016/diff/42002/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:109: self._iteration_num = 0 On 2012/05/09 04:31:47, dennis_jeffrey wrote: > i think this line is unnecessary Thanks. It may be mixed when I rebased. http://codereview.chromium.org/9655016/diff/42002/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:138: def GetDeepMemoryProfileEnv( On 2012/05/09 04:31:47, dennis_jeffrey wrote: > prefix the function name with an underscore if it's meant to be used only > locally to this function. Done. http://codereview.chromium.org/9655016/diff/42002/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:151: result = not_supported On 2012/05/09 04:31:47, dennis_jeffrey wrote: > rather than having a "not_supported" value, should we just raise an exception if > the current platform is not supported? Done. Introduced a new RuntimeError: NotSupportedEnvironmentError. http://codereview.chromium.org/9655016/diff/42002/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:156: def WaitForDeepMemoryProfiler(self): On 2012/05/09 04:31:47, dennis_jeffrey wrote: > prefix the function name with an underscore if it's meant to be used only > locally to this function. Done. http://codereview.chromium.org/9655016/diff/42002/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:157: """Waits for the Deep Memory Profiler finished if running. On 2012/05/09 04:31:47, dennis_jeffrey wrote: > 'finished' --> 'to finish' Done. http://codereview.chromium.org/9655016/diff/42002/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:158: """ On 2012/05/09 04:31:47, dennis_jeffrey wrote: > move this to the previous line if it fits there Done. http://codereview.chromium.org/9655016/diff/42002/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:444: for index in range(self._deep_memory_profile_last_index, On 2012/05/09 04:31:47, dennis_jeffrey wrote: > rather than looping through each index and separately calling > _OutputPerfGraphValue for each one, we could build up a list of tuples and then > output the whole list of tuples in a single call to _OutputPerfGraphValue. Done. Now it prints like following repeatedly: RESULT ControlAttachDetachDOMTree-DMP-TCUsed: DMP-TCMallocUsed= [(0.0,4906.142578125),(20.070510864257812,8683.955078125),(40.041018962860107,6970.5869140625),(60.301534175872803,23050.8740234375),(80.172039747238159,13989.8486328125)] KB seconds
LGTM Awesome! Thanks for addressing all my comments. http://codereview.chromium.org/9655016/diff/42002/chrome/test/functional/perf... File chrome/test/functional/perf_endure.py (right): http://codereview.chromium.org/9655016/diff/42002/chrome/test/functional/perf... chrome/test/functional/perf_endure.py:55: _CHROME_BIN_PATH = os.path.join(perf.BasePerfTest.BrowserPath(), 'chrome') On 2012/05/09 07:44:34, Dai Mikurube wrote: > On 2012/05/09 04:31:47, dennis_jeffrey wrote: > > I think we can just say "self.BrowserPath()" here > > Since it's not in "def something(self)", we can't use self here. I couldn't use > just "BrowserPath()" and "BasePerfTest.BrowserPath()", too.. Ah, you're right!
Thanks, Dennis! Checked the "Commit" box.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmikurube@chromium.org/9655016/49002
Try job failure for 9655016-49002 (retry) on linux_chromeos for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmikurube@chromium.org/9655016/49002
Try job failure for 9655016-49002 (retry) on linux_chromeos for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmikurube@chromium.org/9655016/49002
Change committed as 136248 |
