Add tools/android/memdump/.

Add tools/android/memdump/.

This adds the 'memdump' binary that dumps device memory information for a list
of processes and 'memreport.py' (executed on the host) used to process this
information.

Memdump is similar to /proc/<PID>/smaps except that it is multi-process aware.
For a list of processes (e.g. browser + renderer processes), it is able to
dinstinguish the memory pages that are mapped only in these processes from the
ones that are private or also mapped in external processes.

This is useful to gather high level memory usage information (e.g. heap size vs
code size) for any (non-instrumented) process.

[Philippe, 2013-05-27 15:32:37]

This is not ready for review yet but could this be useful if you want to gather
some data locally.

[Philippe, 2013-05-27 15:32:56]

On 2013/05/27 15:32:37, Philippe wrote:
> This is not ready for review yet but could this be useful if you want to
gather
> some data locally.

s/could this/this could

[bulach, 2013-05-28 11:27:37]

cc: aberent who was looking for something very similar :)

[pliard, 2013-05-28 11:31:35]

On 2013/05/28 11:27:37, bulach wrote:
> cc: aberent who was looking for something very similar :)

Thanks Marcus. Anthony, if you want to use this to profile startup you should
only keep the first part of the program that builds the vector of MemoryMap. The
next part is quite slow (~2secs) due to the syscalls that are made for each
page.

[Philippe, 2013-05-30 09:37:45]

On 2013/05/28 11:31:35, pliard wrote:
> On 2013/05/28 11:27:37, bulach wrote:
> > cc: aberent who was looking for something very similar :)
> 
> Thanks Marcus. Anthony, if you want to use this to profile startup you should
> only keep the first part of the program that builds the vector of MemoryMap.
The
> next part is quite slow (~2secs) due to the syscalls that are made for each
> page.

FYI, this should be ready for review now.

[qsr, 2013-05-30 10:30:25]

https://codereview.chromium.org/15861023/diff/18001/tools/android/memdump/mem...
File tools/android/memdump/memdump.cc (right):

https://codereview.chromium.org/15861023/diff/18001/tools/android/memdump/mem...
tools/android/memdump/memdump.cc:114: bool ReadFromFileAtOffset(int fd, off_t
offset, T* value) {
You might want to use mmap instead of opening your /proc files. You will need to
check if that wokr for your particular files though.

https://codereview.chromium.org/15861023/diff/18001/tools/android/memdump/mem...
tools/android/memdump/memdump.cc:120: if (bytes < 0) {
You could replace 0 by sizeof(*value) if you do not expect partial reads.

https://codereview.chromium.org/15861023/diff/18001/tools/android/memdump/mem...
tools/android/memdump/memdump.cc:315: const pid_t pid = *it;
You could extract this content in another method.

https://codereview.chromium.org/15861023/diff/18001/tools/android/memdump/mem...
tools/android/memdump/memdump.cc:337: auto_resume_processes.Release();
instead of this, you could have it in a scope, and just let the scope ends, that
would also close page_count_fd

[digit, 2013-05-30 10:34:09]

https://codereview.chromium.org/15861023/diff/18001/tools/android/memdump/mem...
File tools/android/memdump/memdump.cc (right):

https://codereview.chromium.org/15861023/diff/18001/tools/android/memdump/mem...
tools/android/memdump/memdump.cc:114: bool ReadFromFileAtOffset(int fd, off_t
offset, T* value) {
From past experience, most files in /proc cannot be mmapped properly :-)

[bulach, 2013-05-30 11:47:07]

just drive-by, feel free to go ahead once others are happy...

however, while at it.. :)
in the internal directory bin/meminfo.py, there's a handy script there that does
some similar stuff, but in python in the host...

would "adb shell cat /proc/PID/maps" and parsing in the host side be too costly
for your tests?
doing as a script in the host would probably be better than requiring users to
build and install the app, wdyt?

https://codereview.chromium.org/15861023/diff/18001/tools/android/memdump/mem...
File tools/android/memdump/memdump.cc (right):

https://codereview.chromium.org/15861023/diff/18001/tools/android/memdump/mem...
tools/android/memdump/memdump.cc:69: static const char kExample[] =
"7fe94ae7d000-7fe94bbd0000 r--p";
nit: instead of strlen below, perhaps:
static const int kExampleLen = sizeof(kExample) - 1;

https://codereview.chromium.org/15861023/diff/18001/tools/android/memdump/mem...
tools/android/memdump/memdump.cc:106: } while (false);
nit: perhaps split into a TokenizeMemoryMapLine() that would return false
instead of break the loop, then here something like:
if (TokenizeMemoryMapLine(..))
  return true;
LOG(ERROR) ....
return false;

https://codereview.chromium.org/15861023/diff/18001/tools/android/memdump/mem...
tools/android/memdump/memdump.cc:112: // Reads sizeof(*value) bytes from file
|fd| at |offset|.
sizeof(T), and in 119 as well?

https://codereview.chromium.org/15861023/diff/18001/tools/android/memdump/mem...
tools/android/memdump/memdump.cc:307: base::Bind(&ResumeProcesses, pids));
s/ResumeProcess/KillAll/, pass the signal in, and remove 304-307 below

https://codereview.chromium.org/15861023/diff/18001/tools/android/memdump/mem...
tools/android/memdump/memdump.cc:337: auto_resume_processes.Release();
On 2013/05/30 10:30:26, qsr wrote:
> instead of this, you could have it in a scope, and just let the scope ends,
that
> would also close page_count_fd

or parhaps:
delete 332, and here:
auto_resume_processes.Release().Run();

https://codereview.chromium.org/15861023/diff/18001/tools/android/memdump/mem...
File tools/android/memdump/memreport.py (right):

https://codereview.chromium.org/15861023/diff/18001/tools/android/memdump/mem...
tools/android/memdump/memreport.py:8: ENTRIES = [
nits: prefix this and all other functions with _ since they're internal to this
module. also, two lines between the functions, and please add copyright notice

[Philippe, 2013-05-30 13:48:43]

Thanks guys!

I have uploaded a new patch set addressing your comments and also fixing a bug
that Ben found by using the tool: we were counting identical (private) pages
mapped multiple times in a same process as many times as they were mapped. They
are now counted only once.

Marcus, the binary running on the device is doing a bit more than parsing the
maps file. It's classifying pages by computing the intersection of page frame
identifiers across all the provided processes. Doing that on the host would
imply that the binary running on the device dumps some information for each
single page.

https://codereview.chromium.org/15861023/diff/18001/tools/android/memdump/mem...
File tools/android/memdump/memdump.cc (right):

https://codereview.chromium.org/15861023/diff/18001/tools/android/memdump/mem...
tools/android/memdump/memdump.cc:69: static const char kExample[] =
"7fe94ae7d000-7fe94bbd0000 r--p";
On 2013/05/30 11:47:07, bulach wrote:
> nit: instead of strlen below, perhaps:
> static const int kExampleLen = sizeof(kExample) - 1;

Done. I got rid of this and addressed when I addressed some comments that
Benjamin made offline.

https://codereview.chromium.org/15861023/diff/18001/tools/android/memdump/mem...
tools/android/memdump/memdump.cc:106: } while (false);
On 2013/05/30 11:47:07, bulach wrote:
> nit: perhaps split into a TokenizeMemoryMapLine() that would return false
> instead of break the loop, then here something like:
> if (TokenizeMemoryMapLine(..))
>   return true;
> LOG(ERROR) ....
> return false;

Good idea. I just moved the error message to the caller site.

https://codereview.chromium.org/15861023/diff/18001/tools/android/memdump/mem...
tools/android/memdump/memdump.cc:112: // Reads sizeof(*value) bytes from file
|fd| at |offset|.
On 2013/05/30 11:47:07, bulach wrote:
> sizeof(T), and in 119 as well?

I made the change here but kept line 119 unchanged if that's ok with you. I find
it safer to use sizeof(*value) since I'm directly writing to value.

https://codereview.chromium.org/15861023/diff/18001/tools/android/memdump/mem...
tools/android/memdump/memdump.cc:120: if (bytes < 0) {
On 2013/05/30 10:30:25, qsr wrote:
> You could replace 0 by sizeof(*value) if you do not expect partial reads.

Done.

https://codereview.chromium.org/15861023/diff/18001/tools/android/memdump/mem...
tools/android/memdump/memdump.cc:307: base::Bind(&ResumeProcesses, pids));
On 2013/05/30 11:47:07, bulach wrote:
> s/ResumeProcess/KillAll/, pass the signal in, and remove 304-307 below

Great idea! I should have thought about it.

https://codereview.chromium.org/15861023/diff/18001/tools/android/memdump/mem...
tools/android/memdump/memdump.cc:315: const pid_t pid = *it;
On 2013/05/30 10:30:26, qsr wrote:
> You could extract this content in another method.

Done.

https://codereview.chromium.org/15861023/diff/18001/tools/android/memdump/mem...
tools/android/memdump/memdump.cc:337: auto_resume_processes.Release();
On 2013/05/30 11:47:07, bulach wrote:
> On 2013/05/30 10:30:26, qsr wrote:
> > instead of this, you could have it in a scope, and just let the scope ends,
> that
> > would also close page_count_fd
> 
> or parhaps:
> delete 332, and here:
> auto_resume_processes.Release().Run();

I followed Benjamin's suggestion. To me it looks less Chromium idiomatic (than
your suggestion Marcus) but might be slightly easier to read.

https://codereview.chromium.org/15861023/diff/18001/tools/android/memdump/mem...
File tools/android/memdump/memreport.py (right):

https://codereview.chromium.org/15861023/diff/18001/tools/android/memdump/mem...
tools/android/memdump/memreport.py:8: ENTRIES = [
On 2013/05/30 11:47:07, bulach wrote:
> nits: prefix this and all other functions with _ since they're internal to
this
> module. also, two lines between the functions, and please add copyright notice

Done.

[qsr, 2013-05-31 10:01:52]

LGTM with nits.

https://codereview.chromium.org/15861023/diff/29001/tools/android/android_too...
File tools/android/android_tools.gyp (right):

https://codereview.chromium.org/15861023/diff/29001/tools/android/android_too...
tools/android/android_tools.gyp:17: 'memdump/memdump.gyp:memdump',
Any reason you want this to build here? Given the comment, it seems that those
are tools needed by the unit tests.

https://codereview.chromium.org/15861023/diff/29001/tools/android/memdump/mem...
File tools/android/memdump/memdump.gyp (right):

https://codereview.chromium.org/15861023/diff/29001/tools/android/memdump/mem...
tools/android/memdump/memdump.gyp:11: '../../..',
Once you depend on base, do you need this?

https://codereview.chromium.org/15861023/diff/29001/tools/android/memdump/mem...
tools/android/memdump/memdump.gyp:15: '../../../third_party/re2/re2.gyp:re2',
Do you need regular expression?

[Philippe, 2013-06-05 09:02:24]

I have just uploaded a final patch set.

https://chromiumcodereview.appspot.com/15861023/diff/29001/tools/android/andr...
File tools/android/android_tools.gyp (right):

https://chromiumcodereview.appspot.com/15861023/diff/29001/tools/android/andr...
tools/android/android_tools.gyp:17: 'memdump/memdump.gyp:memdump',
On 2013/05/31 10:01:53, qsr wrote:
> Any reason you want this to build here? Given the comment, it seems that those
> are tools needed by the unit tests.

Done.

https://chromiumcodereview.appspot.com/15861023/diff/29001/tools/android/memd...
File tools/android/memdump/memdump.gyp (right):

https://chromiumcodereview.appspot.com/15861023/diff/29001/tools/android/memd...
tools/android/memdump/memdump.gyp:11: '../../..',
On 2013/05/31 10:01:53, qsr wrote:
> Once you depend on base, do you need this?

Done.

https://chromiumcodereview.appspot.com/15861023/diff/29001/tools/android/memd...
tools/android/memdump/memdump.gyp:15: '../../../third_party/re2/re2.gyp:re2',
On 2013/05/31 10:01:53, qsr wrote:
> Do you need regular expression?

Done.

[qsr, 2013-06-05 12:31:43]

LGTM with nits.

https://codereview.chromium.org/15861023/diff/40001/tools/android/memdump/mem...
File tools/android/memdump/memreport.py (right):

https://codereview.chromium.org/15861023/diff/40001/tools/android/memdump/mem...
tools/android/memdump/memreport.py:92: # Total stats are not supported yet with
more than two processes.
Maybe you want to display some warnings there.

[digit1, 2013-06-05 12:35:49]

lgtm

[bulach, 2013-06-05 12:40:23]

lgtm, thanks!

[Philippe, 2013-06-05 13:24:52]

Thanks guys!

https://chromiumcodereview.appspot.com/15861023/diff/40001/tools/android/memd...
File tools/android/memdump/memreport.py (right):

https://chromiumcodereview.appspot.com/15861023/diff/40001/tools/android/memd...
tools/android/memdump/memreport.py:92: # Total stats are not supported yet with
more than two processes.
On 2013/06/05 12:31:44, qsr wrote:
> Maybe you want to display some warnings there.

Done.

[Philippe, 2013-06-07 12:23:41]

On 2013/06/05 13:24:52, Philippe wrote:
> Thanks guys!
> 
>
https://chromiumcodereview.appspot.com/15861023/diff/40001/tools/android/memd...
> File tools/android/memdump/memreport.py (right):
> 
>
https://chromiumcodereview.appspot.com/15861023/diff/40001/tools/android/memd...
> tools/android/memdump/memreport.py:92: # Total stats are not supported yet
with
> more than two processes.
> On 2013/06/05 12:31:44, qsr wrote:
> > Maybe you want to display some warnings there.
> 
> Done.

FYI, this was committed as r204254 (my machine crashed during the dcommit and
some post commit hooks were not executed).