Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15093008:
Telemetry: integrates memory_measurement with TCMalloc dumps (Closed)
|
Created:
7 years, 7 months ago by bulach Modified:
7 years, 7 months ago CC:
chromium-reviews, chrome-speed-team+watch_google.com, telemetry+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionTelemetry: integrates memory_measurement with TCMalloc dumps
At the end of each memory_measurement test, force TCMalloc to dump the memory maps, and collect all the files.
BUG=231800
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201953
Patch Set 1 : Patch #
Total comments: 4
Patch Set 2 : comments #
Total comments: 12
Patch Set 3 : TCMallocHeapProfiler #
Total comments: 31
Patch Set 4 : comments #
Total comments: 2
Patch Set 5 : More comments #
Total comments: 2
Patch Set 6 : browser.is_profiler_active #Patch Set 7 : Comments #
Messages
Total messages: 24 (0 generated)
dai: this would allow telemetry to drive tests and fetch dumps that can then be fed to dmp. tonyg: this would duplicate the logs per pid :) and will be fixed as part of https://codereview.chromium.org/14978006/.. both patches can progress in parallel..
Thanks for working on it! Some comments: https://codereview.chromium.org/15093008/diff/2001/tools/telemetry/telemetry/... File tools/telemetry/telemetry/core/platform/profiler/tcmalloc_profiler.py (right): https://codereview.chromium.org/15093008/diff/2001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_profiler.py:13: 'HEAP_PROFILE_TIME_INTERVAL': ('heapprof.timeinterval', 5), s/timeinterval/time_interval/ Since it actually takes 1-2 seconds to dump, dumping every 5 seconds might be too frequent. Also, I have not well tested a case of 100 or more dumps in one execution (I think it works... but just worried). How long is the tests expected to run? https://codereview.chromium.org/15093008/diff/2001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_profiler.py:28: properties['HEAPPROFILE'] = ('heapprof', self._DEFAULT_DEVICE_DIR + 'dmp') I actually prefer an abbr 'dmprof' for code and filenames. :) ('dmp' looks too ambiguous... like just an abbr of "dump" or else.)
thanks dai! comments addressed, another look please? https://codereview.chromium.org/15093008/diff/2001/tools/telemetry/telemetry/... File tools/telemetry/telemetry/core/platform/profiler/tcmalloc_profiler.py (right): https://codereview.chromium.org/15093008/diff/2001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_profiler.py:13: 'HEAP_PROFILE_TIME_INTERVAL': ('heapprof.timeinterval', 5), On 2013/05/14 10:47:56, Dai Mikurube wrote: > s/timeinterval/time_interval/ > > Since it actually takes 1-2 seconds to dump, dumping every 5 seconds might be > too frequent. Also, I have not well tested a case of 100 or more dumps in one > execution (I think it works... but just worried). How long is the tests > expected to run? changed to time_interval. I changed to 20 by default, happy to change to whatever you think it's appropriate :) at this level, we should make no assumptions for the tests themselves, that is, there will be more tests written over time, and there's no control over how long do they run.. https://codereview.chromium.org/15093008/diff/2001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_profiler.py:28: properties['HEAPPROFILE'] = ('heapprof', self._DEFAULT_DEVICE_DIR + 'dmp') On 2013/05/14 10:47:56, Dai Mikurube wrote: > I actually prefer an abbr 'dmprof' for code and filenames. :) ('dmp' looks too > ambiguous... like just an abbr of "dump" or else.) Done.
the latest version not uploaded yet? ;)
On 2013/05/14 14:00:02, Dai Mikurube wrote: > the latest version not uploaded yet? ;) also, I found that profiler_finder.py is updated, and could not rebase straightforward.
uploaded now, sorry for the delay :)
Sorry for later additional comments. https://codereview.chromium.org/15093008/diff/10001/tools/perf/perf_tools/mem... File tools/perf/perf_tools/memory_measurement.py (right): https://codereview.chromium.org/15093008/diff/10001/tools/perf/perf_tools/mem... tools/perf/perf_tools/memory_measurement.py:46: def MeasurePage(self, page, tab, results): Just a question. When is is called? https://codereview.chromium.org/15093008/diff/10001/tools/perf/perf_tools/mem... tools/perf/perf_tools/memory_measurement.py:51: chrome.memoryBenchmarking.isHeapProfilerRunning()) { I'm not confident whether we should always dump when chrome.memoryBenchmarking.isHeapProfilerRunning() is true, or not. How about when --profiler-tool=tcmalloc? https://codereview.chromium.org/15093008/diff/10001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/platform/profiler/profiler_finder.py (right): https://codereview.chromium.org/15093008/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/profiler_finder.py:14: tcmalloc_profiler.TCMallocProfilerFactory, TCMalloc has another profiler than heap profiler: CPU profiler. (Yes, it's a bit confusing... TCMalloc is actually OSS "gperftools" including malloc and a couple of profilers. https://code.google.com/p/gperftools/) We may use the CPU profiler in future, so we may want another name for it. Maybe "TCMalloc*Heap*ProfilerFactory" or like that. https://codereview.chromium.org/15093008/diff/10001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/platform/profiler/tcmalloc_profiler.py (right): https://codereview.chromium.org/15093008/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_profiler.py:75: env_vars['HEAPPROFILE'] = ('heapprof', self._DEFAULT_DIR + 'dmp') also 'dmprof' https://codereview.chromium.org/15093008/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_profiler.py:100: class TCMallocProfilerFactory(profiler.Profiler): Maybe it's a delegate or a proxy rather than a factory?
thanks dai! comments addressed, another look please? https://codereview.chromium.org/15093008/diff/10001/tools/perf/perf_tools/mem... File tools/perf/perf_tools/memory_measurement.py (right): https://codereview.chromium.org/15093008/diff/10001/tools/perf/perf_tools/mem... tools/perf/perf_tools/memory_measurement.py:46: def MeasurePage(self, page, tab, results): On 2013/05/14 15:07:07, Dai Mikurube wrote: > Just a question. When is is called? afaict, this is called after all actions for a given page has been executed.. https://codereview.chromium.org/15093008/diff/10001/tools/perf/perf_tools/mem... tools/perf/perf_tools/memory_measurement.py:51: chrome.memoryBenchmarking.isHeapProfilerRunning()) { On 2013/05/14 15:07:07, Dai Mikurube wrote: > I'm not confident whether we should always dump when > chrome.memoryBenchmarking.isHeapProfilerRunning() is true, or not. How about > when --profiler-tool=tcmalloc? good idea! done. https://codereview.chromium.org/15093008/diff/10001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/platform/profiler/profiler_finder.py (right): https://codereview.chromium.org/15093008/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/profiler_finder.py:14: tcmalloc_profiler.TCMallocProfilerFactory, On 2013/05/14 15:07:07, Dai Mikurube wrote: > TCMalloc has another profiler than heap profiler: CPU profiler. (Yes, it's a > bit confusing... TCMalloc is actually OSS "gperftools" including malloc and a > couple of profilers. https://code.google.com/p/gperftools/) > > We may use the CPU profiler in future, so we may want another name for it. > Maybe "TCMalloc*Heap*ProfilerFactory" or like that. ahn, I didn't know that. :) thanks! added the "Heap". https://codereview.chromium.org/15093008/diff/10001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/platform/profiler/tcmalloc_profiler.py (right): https://codereview.chromium.org/15093008/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_profiler.py:75: env_vars['HEAPPROFILE'] = ('heapprof', self._DEFAULT_DIR + 'dmp') On 2013/05/14 15:07:07, Dai Mikurube wrote: > also 'dmprof' Done. https://codereview.chromium.org/15093008/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_profiler.py:100: class TCMallocProfilerFactory(profiler.Profiler): On 2013/05/14 15:07:07, Dai Mikurube wrote: > Maybe it's a delegate or a proxy rather than a factory? hmm... it's a factory in the sense that it creates the specific profiler, but yeah, since it's not really returning those objects, just made this TCMallocHeapProfiler, it's simpler..
Great patch! Some suggestions inline https://codereview.chromium.org/15093008/diff/18001/tools/perf/perf_tools/mem... File tools/perf/perf_tools/memory_measurement.py (right): https://codereview.chromium.org/15093008/diff/18001/tools/perf/perf_tools/mem... tools/perf/perf_tools/memory_measurement.py:60: print 'TCMalloc heap dumps available at ', dumps I don't like this code in the MemoryMeasurement because it means tcmalloc-heap doesn't work with other measurements. Is there a way we can push this into the Profiler? https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py (right): https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py:18: class _TCMallocHeapProfilerAndroid(profiler.Profiler): Can these just implement object instead of Profiler and drop the is_supported and name methods? It seems like TCMallocHeapProfiler has the Profiler interface covered. https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py:35: adb = adb_commands.AdbCommands(android_device) I didn't double check, but can't we get an existing adb_commands object from the platform_backend or something? https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py:41: print 'Setting device property ', values[0], values[1] logging.info? https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py:45: raise Exception('New device properties were set, run again.') Why do we have to run again? Can we do Browser.close() or something instead? https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py:59: print 'TCMalloc dumps available in the device ', self._DEFAULT_DEVICE_DIR Is this print necessary since it will be pulled to the host? https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py:60: adb = adb_commands.AdbCommands(self._browser_backend.options.android_device) Again, let's try to reuse adb_commands if possible. https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py:74: env_vars = dict(_ENV_VARIABLES) What does the dict() wrapper do? Isn't it already a dict? https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py:85: if not os.path.exists(os.environ['HEAPPROFILE']): Should we check isdir() as well? https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py:118: if (sys.platform != 'linux2'): drop the parens. Also, I think we normally do startswith('linux') instead of == 'linux2'. https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py:120: if options.browser_type.startswith('android'): Do we need to throw in an: if options.browser_type.startswith('cros'): return False https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py:122: return _TCMallocHeapProfilerLinux.is_supported(options) These is_supported calls are no-ops. I'd just return True.
Basically lgtm from my side just with one comment about chrome.memoryBenchmarking.isHeapProfilerRunning. I defer the final approval to tonyg. Thanks! https://codereview.chromium.org/15093008/diff/10001/tools/perf/perf_tools/mem... File tools/perf/perf_tools/memory_measurement.py (right): https://codereview.chromium.org/15093008/diff/10001/tools/perf/perf_tools/mem... tools/perf/perf_tools/memory_measurement.py:46: def MeasurePage(self, page, tab, results): On 2013/05/14 15:34:17, bulach wrote: > On 2013/05/14 15:07:07, Dai Mikurube wrote: > > Just a question. When is is called? > > afaict, this is called after all actions for a given page has been executed.. Got it. Thanks! https://codereview.chromium.org/15093008/diff/10001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/platform/profiler/tcmalloc_profiler.py (right): https://codereview.chromium.org/15093008/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_profiler.py:100: class TCMallocProfilerFactory(profiler.Profiler): On 2013/05/14 15:34:17, bulach wrote: > On 2013/05/14 15:07:07, Dai Mikurube wrote: > > Maybe it's a delegate or a proxy rather than a factory? > > hmm... it's a factory in the sense that it creates the specific profiler, but > yeah, since it's not really returning those objects, just made this > TCMallocHeapProfiler, it's simpler.. Looks good! https://codereview.chromium.org/15093008/diff/18001/tools/perf/perf_tools/mem... File tools/perf/perf_tools/memory_measurement.py (right): https://codereview.chromium.org/15093008/diff/18001/tools/perf/perf_tools/mem... tools/perf/perf_tools/memory_measurement.py:52: if self._is_running_tcmalloc_heap_profiler: Ah, I thought that this condition can coexist with chrome && ... && chrome.memoryBenchmarking.isHeapProfilerRunning().
thanks tony! comments addressed and some clarifications below.. https://codereview.chromium.org/15093008/diff/18001/tools/perf/perf_tools/mem... File tools/perf/perf_tools/memory_measurement.py (right): https://codereview.chromium.org/15093008/diff/18001/tools/perf/perf_tools/mem... tools/perf/perf_tools/memory_measurement.py:60: print 'TCMalloc heap dumps available at ', dumps On 2013/05/14 16:52:44, tonyg wrote: > I don't like this code in the MemoryMeasurement because it means tcmalloc-heap > doesn't work with other measurements. Is there a way we can push this into the > Profiler? "it's complicated" :) - Profiler knows nothing about tab, or browser, and that's probably for good. - tcmalloc will dump the heap at regular intervals, for any measurement, by using --profiler=tcmalloc-heap.. this is just a "minor" optimization to ensure that the _last_ dump is actually made, which seems to make sense in "memory_measurment.py", no? - at most, other measurements would loose up to the last "interval" dump.. so whilst I agree it'd be nice to push this to profiler, would you be ok doing as a separate patch? https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py (right): https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py:18: class _TCMallocHeapProfilerAndroid(profiler.Profiler): On 2013/05/14 16:52:44, tonyg wrote: > Can these just implement object instead of Profiler and drop the is_supported > and name methods? It seems like TCMallocHeapProfiler has the Profiler interface > covered. oh, much nicer! done. https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py:35: adb = adb_commands.AdbCommands(android_device) On 2013/05/14 16:52:44, tonyg wrote: > I didn't double check, but can't we get an existing adb_commands object from the > platform_backend or something? Done. https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py:41: print 'Setting device property ', values[0], values[1] On 2013/05/14 16:52:44, tonyg wrote: > logging.info? Done. https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py:45: raise Exception('New device properties were set, run again.') On 2013/05/14 16:52:44, tonyg wrote: > Why do we have to run again? Can we do Browser.close() or something instead? I think at this stage where the profiler has been created is already too late to mess up with the browser... https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py:59: print 'TCMalloc dumps available in the device ', self._DEFAULT_DEVICE_DIR On 2013/05/14 16:52:44, tonyg wrote: > Is this print necessary since it will be pulled to the host? Done. https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py:60: adb = adb_commands.AdbCommands(self._browser_backend.options.android_device) On 2013/05/14 16:52:44, tonyg wrote: > Again, let's try to reuse adb_commands if possible. Done. https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py:74: env_vars = dict(_ENV_VARIABLES) On 2013/05/14 16:52:44, tonyg wrote: > What does the dict() wrapper do? Isn't it already a dict? at some point I did that in the classmethod and didn't want to mess up with the "singleton", so dict() makes a copy of it.. no longer necessary, removed here and above. https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py:85: if not os.path.exists(os.environ['HEAPPROFILE']): On 2013/05/14 16:52:44, tonyg wrote: > Should we check isdir() as well? adding an assertion below. https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py:118: if (sys.platform != 'linux2'): On 2013/05/14 16:52:44, tonyg wrote: > drop the parens. Also, I think we normally do startswith('linux') instead of == > 'linux2'. Done. https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py:118: if (sys.platform != 'linux2'): On 2013/05/14 16:52:44, tonyg wrote: > drop the parens. Also, I think we normally do startswith('linux') instead of == > 'linux2'. Done. https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py:120: if options.browser_type.startswith('android'): On 2013/05/14 16:52:44, tonyg wrote: > Do we need to throw in an: > > if options.browser_type.startswith('cros'): > return False Done. https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py:122: return _TCMallocHeapProfilerLinux.is_supported(options) On 2013/05/14 16:52:44, tonyg wrote: > These is_supported calls are no-ops. I'd just return True. Done.
lgtm https://codereview.chromium.org/15093008/diff/18001/tools/perf/perf_tools/mem... File tools/perf/perf_tools/memory_measurement.py (right): https://codereview.chromium.org/15093008/diff/18001/tools/perf/perf_tools/mem... tools/perf/perf_tools/memory_measurement.py:53: dumps = eval(tab.EvaluateJavaScript(""" Please use json instead of eval https://codereview.chromium.org/15093008/diff/18001/tools/perf/perf_tools/mem... tools/perf/perf_tools/memory_measurement.py:60: print 'TCMalloc heap dumps available at ', dumps On 2013/05/14 17:54:47, bulach wrote: > On 2013/05/14 16:52:44, tonyg wrote: > > I don't like this code in the MemoryMeasurement because it means tcmalloc-heap > > doesn't work with other measurements. Is there a way we can push this into the > > Profiler? > > "it's complicated" :) > - Profiler knows nothing about tab, or browser, and that's probably for good. > > - tcmalloc will dump the heap at regular intervals, for any measurement, by > using --profiler=tcmalloc-heap.. this is just a "minor" optimization to ensure > that the _last_ dump is actually made, which seems to make sense in > "memory_measurment.py", no? > > - at most, other measurements would loose up to the last "interval" dump.. > > so whilst I agree it'd be nice to push this to profiler, would you be ok doing > as a separate patch? > > How long is the interval? If it is sufficiently short, should we just remove this code from memory_measurement? In any case, it seems like we don't need the print line because the profiler is guaranteed to print that out in collect profiler. Also, if you decide to keep this, please add a comment explaining that this is a minor optimization to ensure the last dump is made. https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py (right): https://codereview.chromium.org/15093008/diff/18001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py:45: raise Exception('New device properties were set, run again.') On 2013/05/14 17:54:47, bulach wrote: > On 2013/05/14 16:52:44, tonyg wrote: > > Why do we have to run again? Can we do Browser.close() or something instead? > > I think at this stage where the profiler has been created is already too late to > mess up with the browser... You are right, too bad. https://codereview.chromium.org/15093008/diff/24001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py (right): https://codereview.chromium.org/15093008/diff/24001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py:103: return True You can remove this branch now.
thanks all! dai: as we chatted, this is now saving a "browser.pid" file to the dump directory, so then we can distinguish the various dump files. tony: all comments addressed. it turns out that the file names are not actually needed, so I removed the "eval", "print", etc.. I commented why that last dump is necessary, ptal.. https://codereview.chromium.org/15093008/diff/18001/tools/perf/perf_tools/mem... File tools/perf/perf_tools/memory_measurement.py (right): https://codereview.chromium.org/15093008/diff/18001/tools/perf/perf_tools/mem... tools/perf/perf_tools/memory_measurement.py:53: dumps = eval(tab.EvaluateJavaScript(""" On 2013/05/14 18:54:13, tonyg wrote: > Please use json instead of eval Done. https://codereview.chromium.org/15093008/diff/18001/tools/perf/perf_tools/mem... tools/perf/perf_tools/memory_measurement.py:60: print 'TCMalloc heap dumps available at ', dumps On 2013/05/14 18:54:13, tonyg wrote: > On 2013/05/14 17:54:47, bulach wrote: > > On 2013/05/14 16:52:44, tonyg wrote: > > > I don't like this code in the MemoryMeasurement because it means > tcmalloc-heap > > > doesn't work with other measurements. Is there a way we can push this into > the > > > Profiler? > > > > "it's complicated" :) > > - Profiler knows nothing about tab, or browser, and that's probably for good. > > > > - tcmalloc will dump the heap at regular intervals, for any measurement, by > > using --profiler=tcmalloc-heap.. this is just a "minor" optimization to ensure > > that the _last_ dump is actually made, which seems to make sense in > > "memory_measurment.py", no? > > > > - at most, other measurements would loose up to the last "interval" dump.. > > > > so whilst I agree it'd be nice to push this to profiler, would you be ok doing > > as a separate patch? > > > > > > How long is the interval? If it is sufficiently short, should we just remove > this code from memory_measurement? In any case, it seems like we don't need the > print line because the profiler is guaranteed to print that out in collect > profiler. Also, if you decide to keep this, please add a comment explaining that > this is a minor optimization to ensure the last dump is made. the interval is relatively large right now. it takes a couple of secs to dump the files, so in reality it has to be >=5secs.. I added a comment explaining this. also, the file names seem indeed irrelevant, so I removed the print, eval, etc.. see more discussion on the underlying layer here: https://codereview.chromium.org/15082004/ https://codereview.chromium.org/15093008/diff/24001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py (right): https://codereview.chromium.org/15093008/diff/24001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/profiler/tcmalloc_heap_profiler.py:103: return True On 2013/05/14 18:54:13, tonyg wrote: > You can remove this branch now. Done.
Still lgtm. :)
lgtm https://codereview.chromium.org/15093008/diff/28002/tools/perf/perf_tools/mem... File tools/perf/perf_tools/memory_measurement.py (right): https://codereview.chromium.org/15093008/diff/28002/tools/perf/perf_tools/mem... tools/perf/perf_tools/memory_measurement.py:52: if self._is_running_tcmalloc_heap_profiler: I think you could remove the member variable and check tab.browser.options.profiler_tool inline here.
thanks tony! addressed your comment in a slightly different way, let me know wdyt and I'll follow up if needed. https://codereview.chromium.org/15093008/diff/28002/tools/perf/perf_tools/mem... File tools/perf/perf_tools/memory_measurement.py (right): https://codereview.chromium.org/15093008/diff/28002/tools/perf/perf_tools/mem... tools/perf/perf_tools/memory_measurement.py:52: if self._is_running_tcmalloc_heap_profiler: On 2013/05/15 15:46:17, tonyg wrote: > I think you could remove the member variable and check > tab.browser.options.profiler_tool inline here. that's actually a good point. browser doesn't expose options though, so rather than doing that, I created a "browser.is_profiler_active(self, name)" , which seems a nicer API to have...
I tried running Telemetry tests with this. What I observed were: browser.pid (it says "30601") x.30545.0002.buckets x.30545.0002.heap ... x.30545.0007.buckets x.30545.0007.heap x.30797.0001.buckets x.30797.0001.heap ... x.30797.0008.buckets x.30797.0008.heap x.30797.maps It looks like 1) x.30545.0001.* and x.30545.maps are missing. 2) No dumps by 30601 which browser.pid says. (1) might be because of clearing /data/local/tmp/heap files after ContentShell starts. (I haven't verified it yet.) For (2), I'm not sure...
On 2013/05/16 11:02:51, Dai Mikurube wrote: > I tried running Telemetry tests with this. What I observed were: > > browser.pid (it says "30601") > x.30545.0002.buckets > x.30545.0002.heap > ... > x.30545.0007.buckets > x.30545.0007.heap > x.30797.0001.buckets > x.30797.0001.heap > ... > x.30797.0008.buckets > x.30797.0008.heap > x.30797.maps > > It looks like > 1) x.30545.0001.* and x.30545.maps are missing. > 2) No dumps by 30601 which browser.pid says. > > (1) might be because of clearing /data/local/tmp/heap files after ContentShell > starts. (I haven't verified it yet.) > > For (2), I'm not sure... strange... I have just collected a bunch of files (I'll send you from my home directory), and they all look correct.. :-/
actually, sorry, I see what you're saying... :) the directory gets wiped out *after* the content shell starts.. :-/ let me try to fix this.
dai: I have addressed the comments, mind taking another look please?
lgtm (while I haven't executed it yet)
thanks dai! I'll put this in the CQ, we can always refine later (this is not used by any bots, so won't break anything)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/15093008/42001
Message was sent while issue was closed.
Change committed as 201953 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
