Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
177003017:
Optimize execution time of the memdump tool (Closed)
|
Created:
6 years, 10 months ago by hyunki Modified:
6 years, 9 months ago Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionOptimize execution time of the memdump tool
In GalaxyNote3 device, the execution time of memdump takes 5.3s.
Among overall time, GetPagesForMemoryMap function took more than 4s.
This patch removes unnecessary divisions and lseek operations for the hot loop in GetPagesForMemoryMap.
It reduced the execution time from 5.3s to 4.7s on the GalaxyNote3.
BUG=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=253792
Patch Set 1 #
Total comments: 6
Patch Set 2 : add 'const' #Messages
Total messages: 14 (0 generated)
I have made a script tool to use memdump periodically to find peak memory usage when running browser. And I found some optimization points. Please take a look. Thank you!
Thanks Hyunki, lgtm! (I only have minor comments) Out of curiosity, are you using the multi-process capabilities of memdump? Because those are mostly the only reason to prefer memdump to cat /proc/<PID>/smaps (which is orders of magnitude faster). https://codereview.chromium.org/177003017/diff/1/tools/android/memdump/memdum... File tools/android/memdump/memdump.cc (right): https://codereview.chromium.org/177003017/diff/1/tools/android/memdump/memdum... tools/android/memdump/memdump.cc:203: off64_t offset = memory_map.start_address / kPageSize; Nit: const off64_t offset Reviewers in Chromium would not make this comment usually, but you will notice here that we use constness extensively :) (mostly for documentation purpose/extra type safety). https://codereview.chromium.org/177003017/diff/1/tools/android/memdump/memdum... tools/android/memdump/memdump.cc:214: offset++; Nit: You don't seem to be using |offset| in the loop so you can drop this line (which also allows |offset| to be const). https://codereview.chromium.org/177003017/diff/1/tools/android/memdump/memdum... tools/android/memdump/memdump.cc:215: ssize_t bytes = read(pagemap_fd, &page_map_entry, sizeof(PageMapEntry)); Nit: sizeof(page_map_entry) would be slightly better. And to add an extra const, you can rewrite this line like this (if it doesn't fit on a single line): const ssize_t bytes = read( pagemap_fd, &page_map_entry, sizeof(page_map_entry));
On 2014/02/26 12:05:47, Philippe wrote: > Thanks Hyunki, lgtm! (I only have minor comments) > > Out of curiosity, are you using the multi-process capabilities of memdump? > Because those are mostly the only reason to prefer memdump to cat > /proc/<PID>/smaps (which is orders of magnitude faster). Yes, I'm using memdump to use the multi-process capabilities. As I mentioned before, memdump can calculate shared memory smartly. > > https://codereview.chromium.org/177003017/diff/1/tools/android/memdump/memdum... > File tools/android/memdump/memdump.cc (right): > > https://codereview.chromium.org/177003017/diff/1/tools/android/memdump/memdum... > tools/android/memdump/memdump.cc:203: off64_t offset = memory_map.start_address > / kPageSize; > Nit: const off64_t offset > > Reviewers in Chromium would not make this comment usually, but you will notice > here that we use constness extensively :) (mostly for documentation > purpose/extra type safety). > > https://codereview.chromium.org/177003017/diff/1/tools/android/memdump/memdum... > tools/android/memdump/memdump.cc:214: offset++; > Nit: You don't seem to be using |offset| in the loop so you can drop this line > (which also allows |offset| to be const). > > https://codereview.chromium.org/177003017/diff/1/tools/android/memdump/memdum... > tools/android/memdump/memdump.cc:215: ssize_t bytes = read(pagemap_fd, > &page_map_entry, sizeof(PageMapEntry)); > Nit: sizeof(page_map_entry) would be slightly better. And to add an extra const, > you can rewrite this line like this (if it doesn't fit on a single line): > > const ssize_t bytes = read( > pagemap_fd, &page_map_entry, sizeof(page_map_entry)); Thank you for the review. I'll update them tomorrow.
On 2014/02/26 13:12:21, hyunki wrote: > On 2014/02/26 12:05:47, Philippe wrote: > > Thanks Hyunki, lgtm! (I only have minor comments) > > > > Out of curiosity, are you using the multi-process capabilities of memdump? > > Because those are mostly the only reason to prefer memdump to cat > > /proc/<PID>/smaps (which is orders of magnitude faster). > > Yes, I'm using memdump to use the multi-process capabilities. > As I mentioned before, memdump can calculate shared memory smartly. > > > > > > https://codereview.chromium.org/177003017/diff/1/tools/android/memdump/memdum... > > File tools/android/memdump/memdump.cc (right): > > > > > https://codereview.chromium.org/177003017/diff/1/tools/android/memdump/memdum... > > tools/android/memdump/memdump.cc:203: off64_t offset = > memory_map.start_address > > / kPageSize; > > Nit: const off64_t offset > > > > Reviewers in Chromium would not make this comment usually, but you will notice > > here that we use constness extensively :) (mostly for documentation > > purpose/extra type safety). > > > > > https://codereview.chromium.org/177003017/diff/1/tools/android/memdump/memdum... > > tools/android/memdump/memdump.cc:214: offset++; > > Nit: You don't seem to be using |offset| in the loop so you can drop this line > > (which also allows |offset| to be const). > > > > > https://codereview.chromium.org/177003017/diff/1/tools/android/memdump/memdum... > > tools/android/memdump/memdump.cc:215: ssize_t bytes = read(pagemap_fd, > > &page_map_entry, sizeof(PageMapEntry)); > > Nit: sizeof(page_map_entry) would be slightly better. And to add an extra > const, > > you can rewrite this line like this (if it doesn't fit on a single line): > > > > const ssize_t bytes = read( > > pagemap_fd, &page_map_entry, sizeof(page_map_entry)); > > Thank you for the review. > I'll update them tomorrow. Great, thanks Hyunki!
https://codereview.chromium.org/177003017/diff/1/tools/android/memdump/memdum... File tools/android/memdump/memdump.cc (right): https://codereview.chromium.org/177003017/diff/1/tools/android/memdump/memdum... tools/android/memdump/memdump.cc:203: off64_t offset = memory_map.start_address / kPageSize; On 2014/02/26 12:05:48, Philippe wrote: > Nit: const off64_t offset > > Reviewers in Chromium would not make this comment usually, but you will notice > here that we use constness extensively :) (mostly for documentation > purpose/extra type safety). Done. https://codereview.chromium.org/177003017/diff/1/tools/android/memdump/memdum... tools/android/memdump/memdump.cc:214: offset++; On 2014/02/26 12:05:48, Philippe wrote: > Nit: You don't seem to be using |offset| in the loop so you can drop this line > (which also allows |offset| to be const). Done. https://codereview.chromium.org/177003017/diff/1/tools/android/memdump/memdum... tools/android/memdump/memdump.cc:215: ssize_t bytes = read(pagemap_fd, &page_map_entry, sizeof(PageMapEntry)); Done.
The CQ bit was checked by hyunki.baik@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hyunki.baik@samsung.com/177003017/20001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests, components_unittests, compositor_unittests, content_browsertests, content_unittests, crypto_unittests, events_unittests, google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by hyunki.baik@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hyunki.baik@samsung.com/177003017/20001
The CQ bit was unchecked by hyunki.baik@samsung.com
The CQ bit was checked by hyunki.baik@samsung.com
Message was sent while issue was closed.
Change committed as 253792 |
