[telemetry] Add support for composable process dumps in memory-infra

tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py

Index: tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py
diff --git a/tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py b/tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py
index 73239230ce3a7e7e40a9406fcd21ac868f0e6023..b49250068d421969938237a709696ee54612755c 100644
--- a/tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py
+++ b/tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py
@@ -8,85 +8,156 @@ from telemetry.timeline import memory_dump_event
 import mock
 
 
-def TestProcessDumpEvent(dump_id='123456ABCDEF', pid=1234, start=0, mmaps=None,
-                         allocators=None):
+def MakeRawMemoryDumpDict(dump_id, pid, start, mmaps=None, allocators=None):
+
   def vm_region(mapped_file, byte_stats):
     return {
         'mf': mapped_file,

[petrcermak, 2016/01/07 18:59:28]

nit: -2 indent

-        'bs': {k: hex(v) for k, v in byte_stats.iteritems()}}
+        'bs': {k: hex(v)

[petrcermak, 2016/01/07 18:59:28]

no need for a line break here

+               for k, v in byte_stats.iteritems()}
+    }
 
   def attrs(sizes):
-    return {'attrs': {k: {'value': hex(v), 'units': 'bytes'}
-               for k, v in sizes.iteritems()}}
+    return {'attrs': {k: {'value': hex(v),

[petrcermak, 2016/01/07 18:59:28]

no need for a line break here

+                          'units': 'bytes'}
+                      for k, v in sizes.iteritems()}}
 
   if allocators is None:
     allocators = {}
 
-  event = {'ph': 'v', 'id': dump_id, 'pid': pid, 'ts': start * 1000,
+  event = {'ph': 'v',

[petrcermak, 2016/01/07 18:59:29]

nit: I think it's more readable to use the following indentation:

event = {
  'ph': 'v',
  ...
  'ts': ...
  'args': {
    'dumps': {
      ...
    }
  }
}

+           'id': dump_id,
+           'pid': pid,
+           'ts': start * 1000,
            'args': {'dumps': {'allocators': {
-               name: attrs(sizes) for name, sizes in allocators.iteritems()}}}}
+               name: attrs(sizes)

[petrcermak, 2016/01/07 18:59:29]

no need for a blank line

+               for name, sizes in allocators.iteritems()
+           }}}}
   if mmaps:
     event['args']['dumps']['process_mmaps'] = {
         'vm_regions': [vm_region(mapped_file, byte_stats)

[petrcermak, 2016/01/07 18:59:28]

nit: this should be indented 2 spaces only

-                       for mapped_file, byte_stats in mmaps.iteritems()]}
+                       for mapped_file, byte_stats in mmaps.iteritems()]
+    }
+
+  return event
 
+
+def TestProcessDumpEvent(dump_id='123456ABCDEF',

[petrcermak, 2016/01/07 18:59:28]

nit: I wouldn't vertically stack the arguments (same for line 50).

+                         pid=1234,
+                         start=0,
+                         mmaps=None,
+                         allocators=None):
+  event = MakeRawMemoryDumpDict(dump_id,
+                                pid,
+                                start,
+                                mmaps=mmaps,
+                                allocators=allocators)
   process = mock.Mock()
   process.pid = event['pid']
-  return memory_dump_event.ProcessMemoryDumpEvent(process, event)
+  memory_dump = memory_dump_event.ProcessMemoryDumpEvent(process, dump_id,
+                                                         event['ts'])
+  memory_dump.AddRawEvent(event)
+  memory_dump.ProcessAllocatorDumps()
+  return memory_dump
 
 
 class ProcessMemoryDumpEventUnitTest(unittest.TestCase):
+
   def testProcessMemoryDump_allocators(self):
-    memory_dump = TestProcessDumpEvent(allocators={
-      'v8': {'size': 10, 'allocated_objects_size' : 5},
-      'v8/allocated_objects': {'size': 4},
-      'skia': {'not_size': 10, 'allocated_objects_size' : 5},
-      'skia/cache1': {'size': 24},
-      'skia/cache2': {'not_size': 20},
-      'skia/cache2/obj1': {'size': 8},
-      'skia/cache2/obj2': {'size': 9},
-      'skia_different/obj': {'size': 30},
-      'skia_different/obj/not_counted': {'size': 26},
-      'global/0xdead': {'size': 26}
-    })
+    process = mock.Mock()
+    process.pid = 1234
+    dump_id = 'abcd'
+    start = 0
+    memory_dump = memory_dump_event.ProcessMemoryDumpEvent(process, dump_id,
+                                                           start)
+    memory_dump.AddRawEvent(
+        MakeRawMemoryDumpDict(dump_id,

[petrcermak, 2016/01/07 18:59:29]

nit: unnecessary vertical stacking of arguments (wastes space, more difficult to
read):

memory_dump.AddRawEvent(MakeRawMemoryDumpDict(
    dump_id, process.pid, start, allocators={
      'v8': ...
    })

+                              process.pid,
+                              start,
+                              allocators={
+                                  'v8': {'size': 10,

[petrcermak, 2016/01/07 18:59:29]

nit: I think that the following is more readable and nice to read:

'v8': {
  'size': 10,
  ...
},
...

+                                         'allocated_objects_size': 5},
+                                  'v8/allocated_objects': {'size': 4},
+                                  'skia': {'not_size': 10,
+                                           'allocated_objects_size': 5},
+                                  'skia/cache1': {'size': 24}
+                              }))
+    memory_dump.AddRawEvent(MakeRawMemoryDumpDict(
+        dump_id,
+        process.pid,
+        start,
+        allocators={
+            'skia/cache2': {'not_size': 20},

[petrcermak, 2016/01/07 18:59:29]

ditto

+            'skia/cache2/obj1': {'size': 8},
+            'skia/cache2/obj2': {'size': 9},
+            'skia_different/obj': {'size': 30},
+            'skia_different/obj/not_counted': {'size': 26},
+            'global/0xdead': {'size': 26}
+        }))
+    memory_dump.ProcessAllocatorDumps()
+
     EXPECTED = {

[petrcermak, 2016/01/07 18:59:28]

nit: EXPECTED_what?

-      'skia': {'allocated_objects_size': 5, 'not_size': 30, 'size': 41},
-      'v8': {'allocated_objects_size': 5, 'size': 10},
-      'skia_different': {'size': 30}}
+        'skia': {'allocated_objects_size': 5,

[petrcermak, 2016/01/07 18:59:28]

ditto

+                 'not_size': 30,
+                 'size': 41},
+        'v8': {'allocated_objects_size': 5,
+               'size': 10},
+        'skia_different': {'size': 30}
+    }
 
     self.assertEquals(memory_dump._allocators, EXPECTED)
 
   def testProcessMemoryDump_mmaps(self):

[petrcermak, 2016/01/07 18:59:29]

It might also be worth checking that everything works if a single dump contains
both mmaps and allocators.

[ssid, 2016/01/07 20:46:09]

Done.

-    ALL = [2 ** x for x in range(8)]
-    (JAVA_SPACES, JAVA_CACHE, ASHMEM, NATIVE_1, NATIVE_2,
-     STACK, FILES_APK, DEVICE_GPU) = ALL
-
-    memory_dump = TestProcessDumpEvent(mmaps={
-      '/dev/ashmem/dalvik-space-foo': {'pss': JAVA_SPACES},
-      '/dev/ashmem/dalvik-jit-code-cache': {'pss': JAVA_CACHE},
-      '/dev/ashmem/other-random-stuff': {'pss': ASHMEM},
-      '[heap] bar': {'pss': NATIVE_1},
-      '': {'pss': NATIVE_2},
-      '[stack thingy]': {'pss': STACK},
-      'my_little_app.apk': {'pss': FILES_APK},
-      '/dev/mali': {'pss': DEVICE_GPU},
-    })
+    ALL = [2**x for x in range(8)]
+    (JAVA_SPACES, JAVA_CACHE, ASHMEM, NATIVE_1, NATIVE_2, STACK, FILES_APK,
+     DEVICE_GPU) = ALL
 
-    EXPECTED = {
-      '/': sum(ALL),
-      '/Android/Java runtime': JAVA_SPACES + JAVA_CACHE,
-      '/Android/Ashmem': ASHMEM,
-      '/Android': JAVA_SPACES + JAVA_CACHE + ASHMEM,
-      '/Native heap': NATIVE_1 + NATIVE_2,
-      '/Stack': STACK,
-      '/Files/apk': FILES_APK,
-      '/Devices': DEVICE_GPU}
+    process = mock.Mock()
+    process.pid = 1234
+    dump_id = 'abcd'
+    start = 0
+    memory_dump = memory_dump_event.ProcessMemoryDumpEvent(process, dump_id,
+                                                           start)
+    memory_dump.AddRawEvent(MakeRawMemoryDumpDict(dump_id,

[petrcermak, 2016/01/07 18:59:29]

I wouldn't stack all the arguments vertically like this (it wastes space and
makes the code a little difficult to read in my opinion):

memory_dump.AddRawEvent(MakeRawMemoryDumpDict(
    dump_id, process.pid, start, allocators={...}))

+                                                  process.pid,
+                                                  start,
+                                                  allocators={
+                                                      'v8': {'size': 10}
+                                                  }))
+    memory_dump.AddRawEvent(MakeRawMemoryDumpDict(
+        dump_id,

[petrcermak, 2016/01/07 18:59:28]

Again, I wouldn't vertically stack the arguments. I find the following much more
visually pleasing:

memory_dump.AddRawEvent(MakeRawMemoryDumpDict(
    dump_id, process.pid, start, mmaps = {
      '/dev/...': ...

+        process.pid,
+        start,
+        mmaps={
+            '/dev/ashmem/dalvik-space-foo': {'pss': JAVA_SPACES},

[petrcermak, 2016/01/07 18:59:28]

You should indent by only 2 spaces in dictionaries and lists (-2 compared to
now)

+            '/dev/ashmem/dalvik-jit-code-cache': {'pss': JAVA_CACHE},
+            '/dev/ashmem/other-random-stuff': {'pss': ASHMEM},
+            '[heap] bar': {'pss': NATIVE_1},
+            '': {'pss': NATIVE_2},
+            '[stack thingy]': {'pss': STACK},
+            'my_little_app.apk': {'pss': FILES_APK},
+            '/dev/mali': {'pss': DEVICE_GPU},
+        }))
+    memory_dump.ProcessAllocatorDumps()
+
+    EXPECTED_ALLOCATORS = {'v8': {'size': 10}}
+    self.assertEquals(memory_dump._allocators, EXPECTED_ALLOCATORS)
 
+    EXPECTED_MMAPS = {
+        '/': sum(ALL),

[petrcermak, 2016/01/07 18:59:28]

ditto (-2 indentation)

+        '/Android/Java runtime': JAVA_SPACES + JAVA_CACHE,
+        '/Android/Ashmem': ASHMEM,
+        '/Android': JAVA_SPACES + JAVA_CACHE + ASHMEM,
+        '/Native heap': NATIVE_1 + NATIVE_2,
+        '/Stack': STACK,
+        '/Files/apk': FILES_APK,
+        '/Devices': DEVICE_GPU
+    }
     self.assertTrue(memory_dump.has_mmaps)
-    for path, value in EXPECTED.iteritems():
-      self.assertEquals(value,
-                        memory_dump.GetMemoryBucket(path).GetValue(
-                            'proportional_resident'))
+    for path, value in EXPECTED_MMAPS.iteritems():
+      self.assertEquals(
+          value,
+          memory_dump.GetMemoryBucket(path).GetValue('proportional_resident'))
 
 
 class MemoryDumpEventUnitTest(unittest.TestCase):
@@ -103,21 +174,18 @@ class MemoryDumpEventUnitTest(unittest.TestCase):
 
     self.assertEquals(
         repr(process_dump1),
-        'ProcessMemoryDumpEvent[pid=1234, allocated_objects_v8=5,'
-        ' allocator_v8=10, mmaps_ashmem=5, mmaps_java_heap=0,'
-        ' mmaps_native_heap=0, mmaps_overall_pss=5, mmaps_private_dirty=0]')
+        'ProcessMemoryDumpEvent[pid=1234, allocated_objects_v8=5, '
+        'allocator_v8=10, mmaps_ashmem=5, mmaps_overall_pss=5]')
     self.assertEquals(
         repr(process_dump2),
-        'ProcessMemoryDumpEvent[pid=1234, allocated_objects_v8=10,'
-        ' allocator_oilpan=40, allocator_v8=20, mmaps_ashmem=0,'
-        ' mmaps_java_heap=0, mmaps_native_heap=42, mmaps_overall_pss=42,'
-        ' mmaps_private_dirty=27]')
+        'ProcessMemoryDumpEvent[pid=1234, allocated_objects_v8=10, '
+        'allocator_oilpan=40, allocator_v8=20, mmaps_native_heap=42, '
+        'mmaps_overall_pss=42, mmaps_private_dirty=27]')
     self.assertEquals(
         repr(global_dump),
-        'GlobalMemoryDump[id=123456ABCDEF, allocated_objects_v8=15,'
-        ' allocator_oilpan=40, allocator_v8=30, mmaps_ashmem=5,'
-        ' mmaps_java_heap=0, mmaps_native_heap=42, mmaps_overall_pss=47,'
-        ' mmaps_private_dirty=27]')
+        'GlobalMemoryDump[id=123456ABCDEF, allocated_objects_v8=15, '
+        'allocator_oilpan=40, allocator_v8=30, mmaps_ashmem=5, '
+        'mmaps_native_heap=42, mmaps_overall_pss=47, mmaps_private_dirty=27]')
 
   def testDumpEventsTiming(self):
     memory_dump = memory_dump_event.GlobalMemoryDump([
@@ -170,10 +238,7 @@ class MemoryDumpEventUnitTest(unittest.TestCase):
     memory_dump = memory_dump_event.GlobalMemoryDump(
         [process_dump1, process_dump2])
     self.assertEquals({'mmaps_overall_pss': 10,
-                       'mmaps_private_dirty': 0,
-                       'mmaps_java_heap': 0,
                        'mmaps_ashmem': 10,
-                       'mmaps_native_heap': 0,
                        'allocator_v8': 30,
                        'allocated_objects_v8': 15},
                       memory_dump.GetMemoryUsage())
@@ -206,8 +271,6 @@ class MemoryDumpEventUnitTest(unittest.TestCase):
 
     self.assertEquals({'mmaps_overall_pss': HEAP,
                        'mmaps_private_dirty': DIRTY,
-                       'mmaps_java_heap': 0,
-                       'mmaps_ashmem': 0,
                        'mmaps_native_heap': HEAP,
                        'allocator_tracing': TRACING_1,
                        'allocator_malloc': MALLOC},