Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
391643002:
Add some convenient metadata in Android's heap profiler dump. (Closed)
|
Created:
6 years, 5 months ago by Dai Mikurube (NOT FULLTIME) Modified:
6 years, 5 months ago Reviewers:
Primiano Tucci (use gerrit) CC:
chromium-reviews, klundberg+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAdd some convenient metadata in Android's heap profiler dump.
BUG=382489
NOTRY=True
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283625
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 13
Patch Set 5 : #
Total comments: 4
Patch Set 6 : ready #Messages
Total messages: 8 (0 generated)
Hi Primiano, Could you take a look when you have time? They're just metadata, but I think they would sometimes help us to analyze. I wondered if we can have a better cmdline, but I'm not sure how. (It's now just like "com.google.android.apps.chrome:sandboxed_process0".)
Hi Dai! Thanks for the patch. Just some comments below. https://codereview.chromium.org/391643002/diff/60001/tools/android/heap_profi... File tools/android/heap_profiler/heap_dump.c (right): https://codereview.chromium.org/391643002/diff/60001/tools/android/heap_profi... tools/android/heap_profiler/heap_dump.c:67: strftime(asc_tm, 64, "%Y-%m-%d %H:%M:%S%z", &local_tm); Can we avoid extracting the time on the device and push it into the comment by the host? I'm considering this for two reasons: - Most of the times, on testing devices, the timezone is screwed (nobody cares about setting that correctly). The host instead should have the correct time. - timer.h in the Android NDK (w.r.t of 64-bit) is sadly in a bad shape for historical reasons. Take a look to src/base/time/time_posix.cc and all the crazy ifdef logic there :) https://codereview.chromium.org/391643002/diff/60001/tools/android/heap_profi... tools/android/heap_profiler/heap_dump.c:112: printf(" \"pagesize\": %d,\n", getpagesize()); I'm curious, Is there any case in which this is expected to return a value != 4096? Also, all the units in this file are expressed in bytes. what is this for? https://codereview.chromium.org/391643002/diff/60001/tools/android/heap_profi... tools/android/heap_profiler/heap_dump.c:275: FILE* fcmdline = fopen(path, "r"); I'd personally just use raw open and read in these simple cases, just to avoid introducing dependencies if there isn't an actual benefit (you don't need to use any actual stdio facility, just read at most N bytes from a file). But I'm fine either ways, choose the one you prefer. I did use fopen and fgets only for the maps because I needed to read the file line-by-line there. https://codereview.chromium.org/391643002/diff/60001/tools/android/heap_profi... tools/android/heap_profiler/heap_dump.c:295: char comment[1024]; What about just: char comment[1024] = { '\0' }; and remove the l ine on 297? https://codereview.chromium.org/391643002/diff/60001/tools/android/heap_profi... tools/android/heap_profiler/heap_dump.c:307: strncpy(comment, optarg, 1024); s/strncpy/strlcpy/. strlcpy is safer and nowadays is supported on all the platforms we care. Also you might want s/1024/sizeof(comment)/. https://codereview.chromium.org/391643002/diff/60001/tools/android/heap_profi... tools/android/heap_profiler/heap_dump.c:308: comment[1023] = '\0'; Precisely. By using strlcpy you don't need this anymore :)
Thanks! Addressed the comments. https://codereview.chromium.org/391643002/diff/60001/tools/android/heap_profi... File tools/android/heap_profiler/heap_dump.c (right): https://codereview.chromium.org/391643002/diff/60001/tools/android/heap_profi... tools/android/heap_profiler/heap_dump.c:67: strftime(asc_tm, 64, "%Y-%m-%d %H:%M:%S%z", &local_tm); On 2014/07/15 23:42:34, Primiano Tucci wrote: > Can we avoid extracting the time on the device and push it into the comment by > the host? > I'm considering this for two reasons: > - Most of the times, on testing devices, the timezone is screwed (nobody cares > about setting that correctly). The host instead should have the correct time. > - timer.h in the Android NDK (w.r.t of 64-bit) is sadly in a bad shape for > historical reasons. Take a look to src/base/time/time_posix.cc and all the crazy > ifdef logic there :) Makes sense especially for the latter. Changed it just to dump an epoch time in seconds. https://codereview.chromium.org/391643002/diff/60001/tools/android/heap_profi... tools/android/heap_profiler/heap_dump.c:112: printf(" \"pagesize\": %d,\n", getpagesize()); On 2014/07/15 23:42:34, Primiano Tucci wrote: > I'm curious, Is there any case in which this is expected to return a value != > 4096? > Also, all the units in this file are expressed in bytes. what is this for? Ah, it may not be required here. I used it in dmprof to align with page frames in /proc/.../pagemap without any assumption, but here, page frames are provided from memdump in plan. I kept it as is since it's not big, but I'd remove it if you want. https://codereview.chromium.org/391643002/diff/60001/tools/android/heap_profi... tools/android/heap_profiler/heap_dump.c:275: FILE* fcmdline = fopen(path, "r"); On 2014/07/15 23:42:34, Primiano Tucci wrote: > I'd personally just use raw open and read in these simple cases, just to avoid > introducing dependencies if there isn't an actual benefit (you don't need to > use any actual stdio facility, just read at most N bytes from a file). > But I'm fine either ways, choose the one you prefer. > I did use fopen and fgets only for the maps because I needed to read the file > line-by-line there. Okay, switched to open/read. https://codereview.chromium.org/391643002/diff/60001/tools/android/heap_profi... tools/android/heap_profiler/heap_dump.c:295: char comment[1024]; On 2014/07/15 23:42:33, Primiano Tucci wrote: > What about just: > char comment[1024] = { '\0' }; > and remove the l ine on 297? Done. https://codereview.chromium.org/391643002/diff/60001/tools/android/heap_profi... tools/android/heap_profiler/heap_dump.c:307: strncpy(comment, optarg, 1024); On 2014/07/15 23:42:34, Primiano Tucci wrote: > s/strncpy/strlcpy/. > strlcpy is safer and nowadays is supported on all the platforms we care. > Also you might want s/1024/sizeof(comment)/. > Done. https://codereview.chromium.org/391643002/diff/60001/tools/android/heap_profi... tools/android/heap_profiler/heap_dump.c:308: comment[1023] = '\0'; On 2014/07/15 23:42:34, Primiano Tucci wrote: > Precisely. By using strlcpy you don't need this anymore :) Acknowledged.
LGTM, thanks for the patch! (just 2 micro nits, please TAL to them before submitting) > Changed it just to dump an epoch time in seconds. SGTM > I kept it as is since it's not big, but I'd remove it if you want. Sounds reasonable, keep it. I was just curious. https://codereview.chromium.org/391643002/diff/60001/tools/android/heap_profi... File tools/android/heap_profiler/heap_dump.c (right): https://codereview.chromium.org/391643002/diff/60001/tools/android/heap_profi... tools/android/heap_profiler/heap_dump.c:275: FILE* fcmdline = fopen(path, "r"); On 2014/07/16 06:24:57, Dai Mikurube wrote: > On 2014/07/15 23:42:34, Primiano Tucci wrote: > > I'd personally just use raw open and read in these simple cases, just to avoid > > introducing dependencies if there isn't an actual benefit (you don't need to > > use any actual stdio facility, just read at most N bytes from a file). > > But I'm fine either ways, choose the one you prefer. > > I did use fopen and fgets only for the maps because I needed to read the file > > line-by-line there. > > Okay, switched to open/read. Thanks :) https://codereview.chromium.org/391643002/diff/80001/tools/android/heap_profi... File tools/android/heap_profiler/heap_dump.c (right): https://codereview.chromium.org/391643002/diff/80001/tools/android/heap_profi... tools/android/heap_profiler/heap_dump.c:64: read_proc_cmdline(cmdline, 512); s/512/sizeof(cmdline)/ https://codereview.chromium.org/391643002/diff/80001/tools/android/heap_profi... tools/android/heap_profiler/heap_dump.c:267: static int read_proc_cmdline(char* cmdline, int size) { you don't seem to make any use of the return value here (which is reasonable for me). I'd just make this void and remove the return.
Updated. Thanks! Committing. https://codereview.chromium.org/391643002/diff/80001/tools/android/heap_profi... File tools/android/heap_profiler/heap_dump.c (right): https://codereview.chromium.org/391643002/diff/80001/tools/android/heap_profi... tools/android/heap_profiler/heap_dump.c:64: read_proc_cmdline(cmdline, 512); On 2014/07/16 16:36:18, Primiano Tucci wrote: > s/512/sizeof(cmdline)/ Done. https://codereview.chromium.org/391643002/diff/80001/tools/android/heap_profi... tools/android/heap_profiler/heap_dump.c:267: static int read_proc_cmdline(char* cmdline, int size) { On 2014/07/16 16:36:18, Primiano Tucci wrote: > you don't seem to make any use of the return value here (which is reasonable > for me). > I'd just make this void and remove the return. Done.
The CQ bit was checked by dmikurube@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmikurube@chromium.org/391643002/100001
Message was sent while issue was closed.
Change committed as 283625 |
