Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
291723002:
Add --trace-memory option for tracing heap memory (Closed)
|
Created:
6 years, 7 months ago by JungJik Modified:
6 years, 6 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd --trace-memory option for tracing heap memory
desktop-chromium can trace memory status in about://tracing.
as same as desktop, this patch enables android to use
disabled-by-default-memory option in adb_profile_chrome script.
BUG=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273031
Patch Set 1 #Patch Set 2 : #
Total comments: 8
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Patch Set 5 : #
Total comments: 1
Patch Set 6 : #
Messages
Total messages: 27 (0 generated)
Please take a look :)
Thanks for the patch. One question below. https://codereview.chromium.org/291723002/diff/20001/build/android/adb_profil... File build/android/adb_profile_chrome.py (right): https://codereview.chromium.org/291723002/diff/20001/build/android/adb_profil... build/android/adb_profile_chrome.py:449: system_properties[_HEAP_PROFILE_MMAP] = 0 Should we reset this property after tracing? Are there any side effects if it is left enabled? If so, would it make sense to add a MemoryTracingController class (similar to ChromeTracingController etc.) that ensure the property is enabled only for the duration of tracing?
https://codereview.chromium.org/291723002/diff/20001/build/android/adb_profil... File build/android/adb_profile_chrome.py (right): https://codereview.chromium.org/291723002/diff/20001/build/android/adb_profil... build/android/adb_profile_chrome.py:449: system_properties[_HEAP_PROFILE_MMAP] = 0 On 2014/05/19 11:39:49, Sami wrote: > Should we reset this property after tracing? Are there any side effects if it is > left enabled? > > If so, would it make sense to add a MemoryTracingController class (similar to > ChromeTracingController etc.) that ensure the property is enabled only for the > duration of tracing? Thanks for your comment. I've just checked the code. if the profiler is stopped, the hooking functions are unset. so whether the flag is enabled or not, it does not effect on browser. ref: heap-profiler.cc/void HeapProfilerStop() http://goo.gl/8OC4mx
https://codereview.chromium.org/291723002/diff/20001/build/android/adb_profil... File build/android/adb_profile_chrome.py (right): https://codereview.chromium.org/291723002/diff/20001/build/android/adb_profil... build/android/adb_profile_chrome.py:34: _HEAP_PROFILE_MMAP = 'heapprof.mmap' nit: could you call this _HEAP_PROFILE_MMAP_PROPERTY instead so it's a little clearer what it is for? https://codereview.chromium.org/291723002/diff/20001/build/android/adb_profil... build/android/adb_profile_chrome.py:446: device.old_interface.EnableAdbRoot() You can just call EnableAdbRoot() unconditionally since it is a no-op if we're already root. https://codereview.chromium.org/291723002/diff/20001/build/android/adb_profil... build/android/adb_profile_chrome.py:449: system_properties[_HEAP_PROFILE_MMAP] = 0 On 2014/05/19 12:08:10, JungJik wrote: > On 2014/05/19 11:39:49, Sami wrote: > > Should we reset this property after tracing? Are there any side effects if it > is > > left enabled? > > > > If so, would it make sense to add a MemoryTracingController class (similar to > > ChromeTracingController etc.) that ensure the property is enabled only for the > > duration of tracing? > > Thanks for your comment. > I've just checked the code. if the profiler is stopped, the hooking functions > are unset. so whether the flag is enabled or not, it does not effect on browser. > ref: heap-profiler.cc/void HeapProfilerStop() http://goo.gl/8OC4mx Thanks for checking. I was thinking more about the case where you record a trace with adb_profile_chrome with --trace-memory and then use some other profiler like telemetry. If we don't reset the flag you might get distorted results in the second run.
https://codereview.chromium.org/291723002/diff/20001/build/android/adb_profil... File build/android/adb_profile_chrome.py (right): https://codereview.chromium.org/291723002/diff/20001/build/android/adb_profil... build/android/adb_profile_chrome.py:34: _HEAP_PROFILE_MMAP = 'heapprof.mmap' On 2014/05/19 16:00:10, Sami wrote: > nit: could you call this _HEAP_PROFILE_MMAP_PROPERTY instead so it's a little > clearer what it is for? sure! https://codereview.chromium.org/291723002/diff/20001/build/android/adb_profil... build/android/adb_profile_chrome.py:446: device.old_interface.EnableAdbRoot() On 2014/05/19 16:00:10, Sami wrote: > You can just call EnableAdbRoot() unconditionally since it is a no-op if we're > already root. thanks for your comment. https://codereview.chromium.org/291723002/diff/20001/build/android/adb_profil... build/android/adb_profile_chrome.py:449: system_properties[_HEAP_PROFILE_MMAP] = 0 On 2014/05/19 16:00:10, Sami wrote: > On 2014/05/19 12:08:10, JungJik wrote: > > On 2014/05/19 11:39:49, Sami wrote: > > > Should we reset this property after tracing? Are there any side effects if > it > > is > > > left enabled? > > > > > > If so, would it make sense to add a MemoryTracingController class (similar > to > > > ChromeTracingController etc.) that ensure the property is enabled only for > the > > > duration of tracing? > > > > Thanks for your comment. > > I've just checked the code. if the profiler is stopped, the hooking functions > > are unset. so whether the flag is enabled or not, it does not effect on > browser. > > ref: heap-profiler.cc/void HeapProfilerStop() http://goo.gl/8OC4mx > > Thanks for checking. I was thinking more about the case where you record a trace > with adb_profile_chrome with --trace-memory and then use some other profiler > like telemetry. If we don't reset the flag you might get distorted results in > the second run. how about reset after pulling the trace file, then we don't need to worry about the second run. I will file new patch.
On 2014/05/19 16:35:36, JungJik wrote: > https://codereview.chromium.org/291723002/diff/20001/build/android/adb_profil... > File build/android/adb_profile_chrome.py (right): > > https://codereview.chromium.org/291723002/diff/20001/build/android/adb_profil... > build/android/adb_profile_chrome.py:34: _HEAP_PROFILE_MMAP = 'heapprof.mmap' > On 2014/05/19 16:00:10, Sami wrote: > > nit: could you call this _HEAP_PROFILE_MMAP_PROPERTY instead so it's a little > > clearer what it is for? > > sure! > > https://codereview.chromium.org/291723002/diff/20001/build/android/adb_profil... > build/android/adb_profile_chrome.py:446: device.old_interface.EnableAdbRoot() > On 2014/05/19 16:00:10, Sami wrote: > > You can just call EnableAdbRoot() unconditionally since it is a no-op if we're > > already root. > thanks for your comment. > > https://codereview.chromium.org/291723002/diff/20001/build/android/adb_profil... > build/android/adb_profile_chrome.py:449: system_properties[_HEAP_PROFILE_MMAP] = > 0 > On 2014/05/19 16:00:10, Sami wrote: > > On 2014/05/19 12:08:10, JungJik wrote: > > > On 2014/05/19 11:39:49, Sami wrote: > > > > Should we reset this property after tracing? Are there any side effects if > > it > > > is > > > > left enabled? > > > > > > > > If so, would it make sense to add a MemoryTracingController class (similar > > to > > > > ChromeTracingController etc.) that ensure the property is enabled only for > > the > > > > duration of tracing? > > > > > > Thanks for your comment. > > > I've just checked the code. if the profiler is stopped, the hooking > functions > > > are unset. so whether the flag is enabled or not, it does not effect on > > browser. > > > ref: heap-profiler.cc/void HeapProfilerStop() http://goo.gl/8OC4mx > > > > Thanks for checking. I was thinking more about the case where you record a > trace > > with adb_profile_chrome with --trace-memory and then use some other profiler > > like telemetry. If we don't reset the flag you might get distorted results in > > the second run. > > how about reset after pulling the trace file, then we don't need to worry about > the second run. I will file new patch. Could you review this patch again?
Thanks for adding the code to reset the property. I've got one more suggestion to make this a little more reliable and better isolated. https://codereview.chromium.org/291723002/diff/30001/build/android/adb_profil... File build/android/adb_profile_chrome.py (right): https://codereview.chromium.org/291723002/diff/30001/build/android/adb_profil... build/android/adb_profile_chrome.py:443: system_properties = device.old_interface.system_properties Could you please move this bit of code to ChromeTracingController.StartTracing() (and add a new constructor parameter for it, e.g., trace_memory)... https://codereview.chromium.org/291723002/diff/30001/build/android/adb_profil... build/android/adb_profile_chrome.py:478: if options.trace_memory: ...and this code to ChromeTracingController.StopTracing()? This way the try...finally construct makes sure the flag is not left on even if something else fails.
On 2014/05/22 15:52:38, Sami wrote: > Thanks for adding the code to reset the property. I've got one more suggestion > to make this a little more reliable and better isolated. > > https://codereview.chromium.org/291723002/diff/30001/build/android/adb_profil... > File build/android/adb_profile_chrome.py (right): > > https://codereview.chromium.org/291723002/diff/30001/build/android/adb_profil... > build/android/adb_profile_chrome.py:443: system_properties = > device.old_interface.system_properties > Could you please move this bit of code to ChromeTracingController.StartTracing() > (and add a new constructor parameter for it, e.g., trace_memory)... > > https://codereview.chromium.org/291723002/diff/30001/build/android/adb_profil... > build/android/adb_profile_chrome.py:478: if options.trace_memory: > ...and this code to ChromeTracingController.StopTracing()? This way the > try...finally construct makes sure the flag is not left on even if something > else fails. Please take a look again. I fixed it. Thank you!
Thanks and sorry for the delay. lgtm!
On 2014/05/27 10:22:38, Sami wrote: > Thanks and sorry for the delay. lgtm! hooray~ thanks for lgtm! :)
The CQ bit was checked by jungjik.lee@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jungjik.lee@samsung.com/291723002/50001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/9399) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/12493)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
The CQ bit was checked by jungjik.lee@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jungjik.lee@samsung.com/291723002/50001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/9413) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/12502)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
Oops, I think this clashed with my recent refactoring: https://codereview.chromium.org/290013006/ I'm sorry about that. Would you mind rebasing this patch?
On 2014/05/27 16:03:31, Sami wrote: > Oops, I think this clashed with my recent refactoring: > https://codereview.chromium.org/290013006/ > > I'm sorry about that. Would you mind rebasing this patch? Of course I've just rebased it. thanks for the notice.
Thanks, lgtm with a tiny tweak. https://codereview.chromium.org/291723002/diff/70001/build/android/chrome_pro... File build/android/chrome_profiler/chrome_controller.py (right): https://codereview.chromium.org/291723002/diff/70001/build/android/chrome_pro... build/android/chrome_profiler/chrome_controller.py:18: categories, ring_buffer, trace_memory): Could you make this "trace_memory=False" to avoid breaking the unit test (build/android/chrome_profiler/run_tests)?
The CQ bit was checked by jungjik.lee@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jungjik.lee@samsung.com/291723002/90001
Message was sent while issue was closed.
Change committed as 273031
Message was sent while issue was closed.
Just noted this CL. I see you're interested in memory profiling. FYI you might be interested in the work in https://codereview.chromium.org/323893002/ and crbug.com/382489. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
