Further enhancements to command_line frontend to memory_inspector

Further enhancements to command_line frontend to memory_inspector

Print memory maps for a process
Print consolidated memory map data for a process

BUG=340294
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265655

[paulwhitton, 2014-04-16 11:54:20]

Hi Primiano,

Please can you review my changes to the memory inspector command line.

Cheers

Paul

[Primiano Tucci (use gerrit), 2014-04-16 12:54:54]

Thanks Paul,
just a couple of comments, but looks in good shape.

Ah another thing: next time before you git cl upload, take an eye to the output
of the presubmit step, it ususally shows common mistakes like wrapping after 80
cols etc.

https://codereview.chromium.org/240023003/diff/1/tools/memory_inspector/memor...
File tools/memory_inspector/memory_inspector/frontends/command_line.py (right):

https://codereview.chromium.org/240023003/diff/1/tools/memory_inspector/memor...
tools/memory_inspector/memory_inspector/frontends/command_line.py:41: if args[0]
not in ('devices', 'ps', 'stats', 'mmaps',
Hmm are you really sure you need this?
Can't you do something like:

if ..'devices':
 ...

device selection logic here
numer_of_devices...
...

if 'stats':
 ...
elif 'classified_mmaps':
 ....
else
 print 'Invalid command'
 parser.print_help()

https://codereview.chromium.org/240023003/diff/1/tools/memory_inspector/memor...
tools/memory_inspector/memory_inspector/frontends/command_line.py:170:
_PrintProcessMmapsHeadingLines()
Can you avoid making this a separate function and copying the code in here
directly? Seems like _PrintProcessMmapsHeadingLines is called only once, so
probably shouldn't be a function.

https://codereview.chromium.org/240023003/diff/1/tools/memory_inspector/memor...
tools/memory_inspector/memory_inspector/frontends/command_line.py:173:
_PrintProcessMmaps(entry)
Same here: just keep the code here, do not add the extra function.

https://codereview.chromium.org/240023003/diff/1/tools/memory_inspector/memor...
tools/memory_inspector/memory_inspector/frontends/command_line.py:192: try:
Instead of try/catch you could just do:
if not os.path.exists(mmap_rule):
  print ....
  return

https://codereview.chromium.org/240023003/diff/1/tools/memory_inspector/memor...
tools/memory_inspector/memory_inspector/frontends/command_line.py:198:
cls=serialization.Encoder)
In this case you should either indent to the same level of "classified", or push
classified down and align both @ +4 (go for the former)

https://codereview.chromium.org/240023003/diff/1/tools/memory_inspector/memor...
tools/memory_inspector/memory_inspector/frontends/command_line.py:216: def
_PrintProcessStatsHeadingLine():
As said above, it doesn't feel like these functions are really needed, as they
are called only once.

[paulwhitton, 2014-04-22 10:55:55]

Hi Primiano,

Please can you review the changes I have made following your review.

Cheers

Paul

[Primiano Tucci (use gerrit), 2014-04-22 11:24:47]

On 2014/04/22 10:55:55, paulwhitton wrote:
> Hi Primiano,
> 
> Please can you review the changes I have made following your review.
> 
> Cheers
> 
> Paul

Sure, but I think you forgot to upload the CL :-)
I still see only the original patchset.

[Primiano Tucci (use gerrit), 2014-04-22 13:08:48]

THanks Paul.
Looks in good shape, but there is some extra code it might be worth removing
before landing this. See my comments inline.
Also please fix the commit message in the codereview website: there are some
extra "mmaps... work in progress" below BUG=, you can just remove :) (use the
edit issue link on the left)
Thanks for your patience and for your work.

https://codereview.chromium.org/240023003/diff/20001/tools/memory_inspector/m...
File tools/memory_inspector/memory_inspector/frontends/command_line.py (right):

https://codereview.chromium.org/240023003/diff/20001/tools/memory_inspector/m...
tools/memory_inspector/memory_inspector/frontends/command_line.py:42: if args[0]
not in COMMANDS:
I'd just combine this check with the previous one, as follows:
if not args or args[0] not in COMMANDS:
  parser.print_help()
  return -1

https://codereview.chromium.org/240023003/diff/20001/tools/memory_inspector/m...
tools/memory_inspector/memory_inspector/frontends/command_line.py:73: 
Since in all the other cases below, you have always this piece of code:
---
if not options.process_id:
  print 'You need to provide --process_id'
  return -1
---

I'd suggest to do this just once here (before if args == 'stats'), and remove
all the if ...process_id...else from below.

Also, since all the actions below require a "process" object you can retrieve
the process here, and avoid repeating stuff like "device =
backends.GetDevice(backend_name, device_id)" below.
In essence your code here could look like

---
device = backends.GetDevice(backend_name, device_id)
if not device:
 ...omissis... print error and return
device.Initialize()

if args[0] == 'ps':
  ... omissis... copy current code (_ListProcesses and return)

if not options.process_id:
  print 'You need to provide --process_id'
  return -1

process = device.GetProcess(options.process_id)
---

At this point, you don't need to pass backend, device_id and process_id to your
functions (_ListProcessStats, _ListDevices,  _ListProcessMmaps,
_ListProcessClassifiedMmaps).
You just need to pass them the process object, so they can skip any check and
use that directly.

Hint: if you need also the device object in some of those functions, remember
that once you have a process, you can derive the device object as process.device
(i.e. no need to pass "device" if you pass process in the arguments)

Does it make sense?

https://codereview.chromium.org/240023003/diff/20001/tools/memory_inspector/m...
tools/memory_inspector/memory_inspector/frontends/command_line.py:83: if args[0]
== 'mmaps':
nit: this (and below) should be elif (won't make any difference functionally but
is a bit cleaner)

https://codereview.chromium.org/240023003/diff/20001/tools/memory_inspector/m...
tools/memory_inspector/memory_inspector/frontends/command_line.py:88:
_ListProcessMmaps(options.backend, device_id,
Nit: I don't recall all details of the code style, but I think you might need a
new line between backend, and device_id.

https://codereview.chromium.org/240023003/diff/20001/tools/memory_inspector/m...
tools/memory_inspector/memory_inspector/frontends/command_line.py:113: device =
backends.GetDevice(backend_name, device_id)
as told above, you don't need all these 3 lines here. Just pass device (instead
of backend_name and device_id) and filter_proceess name, so you can skip
checking for the device.

https://codereview.chromium.org/240023003/diff/20001/tools/memory_inspector/m...
tools/memory_inspector/memory_inspector/frontends/command_line.py:137: device =
backends.GetDevice(backend_name, device_id)
Same here: if you take (process) as only argument, you don't need all this
if...then...else here, neither the "if not process"... below.

https://codereview.chromium.org/240023003/diff/20001/tools/memory_inspector/m...
tools/memory_inspector/memory_inspector/frontends/command_line.py:163: device =
backends.GetDevice(backend_name, device_id)
And the same here.

https://codereview.chromium.org/240023003/diff/20001/tools/memory_inspector/m...
tools/memory_inspector/memory_inspector/frontends/command_line.py:190: device =
backends.GetDevice(backend_name, device_id)
And here.

https://codereview.chromium.org/240023003/diff/20001/tools/memory_inspector/m...
tools/memory_inspector/memory_inspector/frontends/command_line.py:224: def
_PrintProcessMmaps(entry):
You look to be using this function only in one place (in line 182).
Remove this function and move the print there.

[paulwhitton, 2014-04-23 15:28:21]

Hi Primiano,

Please can you review my further changes.

Cheers

Paul

[Primiano Tucci (use gerrit), 2014-04-23 15:36:57]

LGTM with few nits.
Please take a look to the comments and update the CL before committing.
You just missed two small things.

https://codereview.chromium.org/240023003/diff/40001/tools/memory_inspector/m...
File tools/memory_inspector/memory_inspector/frontends/command_line.py (right):

https://codereview.chromium.org/240023003/diff/40001/tools/memory_inspector/m...
tools/memory_inspector/memory_inspector/frontends/command_line.py:41: elif
args[0] not in COMMANDS:
Hmm, I actually meant combining the 6 lines in one.
Re-read my comment, just replace lines 38-43 with the one I proposed ;-)

https://codereview.chromium.org/240023003/diff/40001/tools/memory_inspector/m...
tools/memory_inspector/memory_inspector/frontends/command_line.py:72: 
As I suggested in my previous comment, you should perform the
device.Initialize() call here.

https://codereview.chromium.org/240023003/diff/40001/tools/memory_inspector/m...
tools/memory_inspector/memory_inspector/frontends/command_line.py:80:
device.Initialize()
Remove this.

https://codereview.chromium.org/240023003/diff/40001/tools/memory_inspector/m...
tools/memory_inspector/memory_inspector/frontends/command_line.py:99:
device.Initialize()
Remove this (move up to line 72)

[paulwhitton, 2014-04-23 15:53:34]

The CQ bit was checked by paulwhitton@google.com

[commit-bot: I haz the power, 2014-04-23 15:53:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/paulwhitton@google.com/240023003/60001

[commit-bot: I haz the power, 2014-04-23 15:54:43]

Change committed as 265655