Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1553183002:
[telemetry] Add support for composable process dumps in memory-infra (Closed)
Description[telemetry] Add support for composable process dumps in memory-infra
The memory-infra process dumps can be split across multiple process
memory dump events in a trace. Telemetry should support merging of them.
BUG=461788
Committed: https://crrev.com/e08757506faee7da1bcb7f9b6f8f1d6a8540f038
Cr-Commit-Position: refs/heads/master@{#369428}
Patch Set 1 #
Total comments: 3
Patch Set 2 : aggregating events in ProcessDump. #
Total comments: 32
Patch Set 3 : adding test #
Total comments: 31
Patch Set 4 : Fix style + unittest #Patch Set 5 : Nits. #
Total comments: 31
Patch Set 6 : Fixes. #Patch Set 7 : Nits. #Patch Set 8 : Nits. #
Total comments: 27
Patch Set 9 : Fixes. #
Total comments: 11
Patch Set 10 : Fixes. #
Messages
Total messages: 33 (10 generated)
ssid@chromium.org changed reviewers: + primiano@chromium.org
PTAL, Thanks.
primiano@chromium.org changed reviewers: + petrcermak@chromium.org
Petr, can you take this review? Thanks!
Here are a few comments. Please add tests for the new functionality (feel free to copy https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu...). Thanks, Petr https://codereview.chromium.org/1553183002/diff/1/tools/telemetry/telemetry/t... File tools/telemetry/telemetry/timeline/memory_dump_event.py (right): https://codereview.chromium.org/1553183002/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:209: assert not self._buckets You probably also want to 'assert not self.has_mmaps'. https://codereview.chromium.org/1553183002/diff/1/tools/telemetry/telemetry/t... File tools/telemetry/telemetry/timeline/trace_event_importer.py (right): https://codereview.chromium.org/1553183002/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/trace_event_importer.py:199: memory_dump = memory_dump_event.ProcessMemoryDumpEvent(process, event) I wonder whether it wouldn't be better to create an empty ProcessMemoryDumpEvent first and then add the values to it instead. Something along the lines of: def _ProcessMemoryDumpEvent(self, event): process = self._GetOrCreateProcess(event['pid']) memory_dump = self._GetOrCreateProcessMemoryDumpEvent(process, event['id']) memory_dump.AddRawEvent(event) process.AddMemoryDumpEvent(memory_dump) This is closer to the Trace Viewer implementation (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu...) and it avoids the need to create multiple ProcessMemoryDumpEvent objects. https://codereview.chromium.org/1553183002/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/trace_event_importer.py:202: return dump.MergeValuesFrom(memory_dump) Unless this is a common pattern in Python (explicitly returning None instead of nothing), I think that the following would be more readable: dump.MergeValuesFrom(memory_dump) return This way, it would be clear that the method doesn't return anything.
A few title/description nits: * We use the term "composable dumps" (rather than "split dumps") in Trace Viewer. * s/multiple dumps/multiple process memory dump events in a trace/ * s/of process dump events/them/ Thanks, Petr
Description was changed from ========== [telemetry] Add support for split process dumps in memory-infra The memory-infra process dumps can be split across multiple dumps. Telemetry should support merging of process dump events. BUG=461788 ========== to ========== [telemetry] Add support for composable process dumps in memory-infra The memory-infra process dumps can be split across multiple process memory dump events in a trace. Telemetry should support merging of them. ==========
Patchset #2 (id:20001) has been deleted
I haven't added test yet. What do you think about this patch? I can't just add a GetOrCreateProcessDump() method since the dumps need to be aggregated before processing. In previous case the allocator total from one dump will replace other if both the dumps have the same allocator names. But if I add events and process multiple times, the totals will get added twice and maybe double counted because I don't have the parent paths anymore. (I still want to keep the aggregation to just use the parent path and not store everything). This would also cause a problem is "malloc" comes in one part and "tracing" comes in another. This shouldn't happen but feels like a bad assumption. So, the allocator needs to be merged first and then processed. For this reason, the processEvent first adds just raw events and then processes the dumps.
https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/memory_dump_event.py (right): https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:285: if result: This is made so that desktop doesn't show ashmem and java heap with empty values.
petrcermak@chromium.org changed reviewers: + perezju@chromium.org
Looks good overall. Here are a few more comments. perezju@: Since you wrote the initial implementation of the ProcessMemoryDumpEvent class, please have a quick look at this. Thanks, Petr https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/memory_dump_event.py (right): https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:156: self._allocators = {} I think that you could set allocators and buckets to None here (so that they raise an exception if someone tries to access them prematurely) and set them correctly in ProcessAllocatorDumps https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:182: """Adds the allocator dumps and vm regions from the timeline event without nit: "Add" (consistency with the rest of the file) https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:182: """Adds the allocator dumps and vm regions from the timeline event without nit: The first sentence in a docstring in Python should typically fit on a single line. https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:189: pass # it's ok if any of those keys are not present nit: Start with a capital letter and add a full stop at the end. https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:197: pass # it's ok if any of those keys are not present ditto https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:200: """Processes the raw memory dump data into interesting stats. nit: "Process" (for consistency with the rest of the file) https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:200: """Processes the raw memory dump data into interesting stats. nit: "interesting" doesn't really mean anything. Maybe "structured"? https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:202: Aggregates and stores the allocator dumps values and the vm regions such I'd say "Parses and aggregates" ("stores" seems automatic). s/such that/so that/ https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:203: that it is easy to retrieve memory usage values to be added in telemetry nit: "retrieve and add memory usage values to telemetry results." https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:204: results. This method needs to be called before making calls to GetMemory* In that case, the GetMemory* methods should fail if they are called too early, either directly (raise an explicit exception) or indirectly (iterate a None). https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:233: pass # it's ok if any of those keys are not present Since you're already touching this line, capital letter and full stop https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:234: self._raw_allocator_dumps = {} Setting this to None might be better - we probably want to fail if someone tries to add an allocator dump after the dump has been finalized. https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:238: self._raw_vm_regions = [] ditto (None) https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:285: if result: On 2016/01/05 22:05:22, ssid wrote: > This is made so that desktop doesn't show ashmem and java heap with empty > values. Acknowledged. https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/trace_event_importer.py (right): https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/trace_event_importer.py:202: memory_dump = global_dumps[event['id']].setdefault( This constructs a ProcessMemoryDumpEvent every single time (plus it's a little difficult to read). I'd go for the following: global_dump = global_dumps[event['id']] if process not in global_dump: global_dump[process] = ... memory_dump = global_dump[process] https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/trace_event_importer.py:251: # Process memory dumps of a process with the same id need to be merged nit: No need to say "Process" at the beginning of the sentence since you later say "of a process". https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/trace_event_importer.py:251: # Process memory dumps of a process with the same id need to be merged nit: To make it easier to understand (i.e. avoid confusion with "dump id"), I'd say "pid".
Description was changed from ========== [telemetry] Add support for composable process dumps in memory-infra The memory-infra process dumps can be split across multiple process memory dump events in a trace. Telemetry should support merging of them. ========== to ========== [telemetry] Add support for composable process dumps in memory-infra The memory-infra process dumps can be split across multiple process memory dump events in a trace. Telemetry should support merging of them. BUG=461788 ==========
I have added tests. could you please take another look? Thanks https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/memory_dump_event.py (right): https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:156: self._allocators = {} On 2016/01/06 18:45:52, petrcermak wrote: > I think that you could set allocators and buckets to None here (so that they > raise an exception if someone tries to access them prematurely) and set them > correctly in ProcessAllocatorDumps Done. https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:182: """Adds the allocator dumps and vm regions from the timeline event without On 2016/01/06 18:45:51, petrcermak wrote: > nit: The first sentence in a docstring in Python should typically fit on a > single line. Done. https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:189: pass # it's ok if any of those keys are not present On 2016/01/06 18:45:51, petrcermak wrote: > nit: Start with a capital letter and add a full stop at the end. Done. https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:197: pass # it's ok if any of those keys are not present On 2016/01/06 18:45:51, petrcermak wrote: > ditto Done. https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:200: """Processes the raw memory dump data into interesting stats. On 2016/01/06 18:45:51, petrcermak wrote: > nit: "Process" (for consistency with the rest of the file) Done. https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:202: Aggregates and stores the allocator dumps values and the vm regions such On 2016/01/06 18:45:51, petrcermak wrote: > I'd say "Parses and aggregates" ("stores" seems automatic). > > s/such that/so that/ Done. https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:203: that it is easy to retrieve memory usage values to be added in telemetry On 2016/01/06 18:45:51, petrcermak wrote: > nit: "retrieve and add memory usage values to telemetry results." Done. https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:204: results. This method needs to be called before making calls to GetMemory* On 2016/01/06 18:45:52, petrcermak wrote: > In that case, the GetMemory* methods should fail if they are called too early, > either directly (raise an explicit exception) or indirectly (iterate a None). Done. https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:233: pass # it's ok if any of those keys are not present On 2016/01/06 18:45:52, petrcermak wrote: > Since you're already touching this line, capital letter and full stop Done. https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:234: self._raw_allocator_dumps = {} On 2016/01/06 18:45:52, petrcermak wrote: > Setting this to None might be better - we probably want to fail if someone tries > to add an allocator dump after the dump has been finalized. Done. https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:238: self._raw_vm_regions = [] On 2016/01/06 18:45:52, petrcermak wrote: > ditto (None) Done. https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/trace_event_importer.py (right): https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/trace_event_importer.py:202: memory_dump = global_dumps[event['id']].setdefault( On 2016/01/06 18:45:52, petrcermak wrote: > This constructs a ProcessMemoryDumpEvent every single time (plus it's a little > difficult to read). I'd go for the following: > > global_dump = global_dumps[event['id']] > if process not in global_dump: > global_dump[process] = ... > memory_dump = global_dump[process] I expected it to create only when it is null. Thanks https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/trace_event_importer.py:251: # Process memory dumps of a process with the same id need to be merged On 2016/01/06 18:45:52, petrcermak wrote: > nit: No need to say "Process" at the beginning of the sentence since you later > say "of a process". Done. https://codereview.chromium.org/1553183002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/trace_event_importer.py:251: # Process memory dumps of a process with the same id need to be merged On 2016/01/06 18:45:52, petrcermak wrote: > nit: To make it easier to understand (i.e. avoid confusion with "dump id"), I'd > say "pid". Oh i meant dump id here. fixed. "of a process" already means same pid.
A few more (mostly style) comments. Thanks, Petr https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/memory_dump_event.py (right): https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:179: return '%s[%s]' % (type(self).__name__, values) idea: There's no reason to put the square brackets here because the string representation of list already contains them: < '[%s]' % [1, 2, 3] > '[[1, 2, 3]]' https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:182: """Add the raw event without processing. supernit: s/the/a/ https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:185: processing.""" The three quotes should be on a separate line. https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:206: This method needs to be called before making calls to GetMemory* functions nit: s/functions/methods/ https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py (right): https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:15: 'mf': mapped_file, nit: -2 indent https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:16: 'bs': {k: hex(v) no need for a line break here https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:21: return {'attrs': {k: {'value': hex(v), no need for a line break here https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:28: event = {'ph': 'v', nit: I think it's more readable to use the following indentation: event = { 'ph': 'v', ... 'ts': ... 'args': { 'dumps': { ... } } } https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:33: name: attrs(sizes) no need for a blank line https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:38: 'vm_regions': [vm_region(mapped_file, byte_stats) nit: this should be indented 2 spaces only https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:45: def TestProcessDumpEvent(dump_id='123456ABCDEF', nit: I wouldn't vertically stack the arguments (same for line 50). https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:74: MakeRawMemoryDumpDict(dump_id, nit: unnecessary vertical stacking of arguments (wastes space, more difficult to read): memory_dump.AddRawEvent(MakeRawMemoryDumpDict( dump_id, process.pid, start, allocators={ 'v8': ... }) https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:78: 'v8': {'size': 10, nit: I think that the following is more readable and nice to read: 'v8': { 'size': 10, ... }, ... https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:90: 'skia/cache2': {'not_size': 20}, ditto https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:99: EXPECTED = { nit: EXPECTED_what? https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:100: 'skia': {'allocated_objects_size': 5, ditto https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:110: def testProcessMemoryDump_mmaps(self): It might also be worth checking that everything works if a single dump contains both mmaps and allocators. https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:121: memory_dump.AddRawEvent(MakeRawMemoryDumpDict(dump_id, 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={...})) https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:128: dump_id, 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/...': ... https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:132: '/dev/ashmem/dalvik-space-foo': {'pss': JAVA_SPACES}, You should indent by only 2 spaces in dictionaries and lists (-2 compared to now) https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:147: '/': sum(ALL), ditto (-2 indentation) https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/trace_event_importer.py (right): https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/trace_event_importer.py:203: global_dumps[event['id']][ this might look a bit better as follows: global_dumps[event['id']][process] = ( memory_dump_event... https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py (right): https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py:1072: processes = set([52, 54]) nit: expected_processes https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py:1073: expected = [['1234ABCD', 0, 11], ['1234ABDF', 122, 11]] nit: expected_WHAT? https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py:1090: 'args': {'dumps': {'allocators': {'v8': {'attrs': {'size': again, please break lines and indent consistently: 'args': { 'dumps': { 'allocators': { ... } } } it's much more readable and it looks better in my opinion. https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py:1105: expected = [[10, 11], [12, 13]] nit: expected_WHAT? https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py:1108: memory_dump = m.IterGlobalMemoryDumps() the next(...) call should be on this line already https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py:1118: nit: There should only be one blank line here
Hm, I tried to use google python formatter and it turned out this disastrous. Fixed the style as much as I could. PTAL, thanks. https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/memory_dump_event.py (right): https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:179: return '%s[%s]' % (type(self).__name__, values) On 2016/01/07 18:59:28, petrcermak wrote: > idea: There's no reason to put the square brackets here because the string > representation of list already contains them: > > < '[%s]' % [1, 2, 3] > > '[[1, 2, 3]]' discussed offline. https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py (right): https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:110: def testProcessMemoryDump_mmaps(self): On 2016/01/07 18:59:29, petrcermak wrote: > It might also be worth checking that everything works if a single dump contains > both mmaps and allocators. Done. https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py (right): https://codereview.chromium.org/1553183002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py:1105: expected = [[10, 11], [12, 13]] On 2016/01/07 18:59:29, petrcermak wrote: > nit: expected_WHAT? Leaving this consistent with the rest of the file. Specifying _what only when multiple expected are defined.
LGTM with final nits :-) Thanks, Petr https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py (right): https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:20: for k, v in sizes.iteritems()}} nit: 'for' should be aligned with 'k:': return {'attrs': {k: {'value': hex(v), 'units': 'bytes'} for k, v in sizes.iteritems()}} ^ | https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:58: memory_dump.AddRawEvent(MakeRawMemoryDumpDict(dump_id, process.pid, start, nit: In Chromium Python, one of the following two indentation styles should be used: Option A - break immediately after opening bracket and indent 4 spaces MakeRawMemoryDumpDict( dump_id, process.pid, start, allocators={ ... Option B - align vertically after the opening bracket: MakeRawMemoryDumpDict(dump_id, process.pid, start, allocators={ ... Option A is what you want here (so that the allocators wouldn't be indented too much). https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:67: allocators={ ditto https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:78: 'skia': {'allocated_objects_size': 5, I would personally either write the objects on a single line: 'skia': {'allocated_objects_size': 5, ...}, 'v8': {...}, or vertically expand and, consequently, align them: 'skia': { 'allocated_objects_size': 5, ... }, 'v8': { ... } https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py (right): https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py:1090: {'name': 'a', 'cat': 'b', 'ph': 'v', 'pid': 52, 'ts': 123, 'id': '123', This looks much better than before, but I would personally visually expand each completely (for the sake of readability and ease of modification) events = [ { 'name': 'a', 'cat': 'b', ... 'args': { 'dumps': { 'allocators': { ...
I don't think this is quite ready yet. Also you'll need a telemetry owner to do the final approve. https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/memory_dump_event.py (right): https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:141: timing correlated with that of other events in the model. I'm not all that sold on the idea of fragmenting the constructor into: construct + AddRawEvent's + ProcessAllocatorDumps. I think that a better solution could be for the caller to do the grouping of events beforehand and then call the constructor as ProcessMemoryDumpEvent(process, list_of_raw_events_for_this_process). If you decide to stick with the fragmented constructor, that would be OK but: - add to this docstring an example of how to create and populate this object. - make sure to complain if calling other methods before ProcessAllocatorDumps. - make sure to complain if calling AddRawEvent *after* ProcessAllocatorDumps. - make sure to complain if ProcessAllocatorDumps is called but no raw events were added. - add unittests to check for these conditions. https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:186: """ This method should complain if you've already called ProcessAllocatorDumps. https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:187: assert event['ph'] == 'v' and self.process.pid == event['pid'] should also check that self.dump_id and event['id'] match. https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:198: self.has_mmaps = True Move setting the value of has_mmaps to the processing method below. Also (at least from the old implementation) it should be False if the 'vm_regions' key is in the dictionary but has an empty value. (Or maybe that never happens?) https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:205: Parse and aggregate the allocator dumps values and the vm regions so that If this processes both allocator dumps *and* vm regions, then the name of the method is misleading. Maybe just "ProcessRawEvents()" or ...? https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:291: usage[key] = result Hmm.. I'm not super happy about this. We should have a way to distinguish between that metric not being available (so it's not returned in the results), from the metric being available *and* having a value of 0. Although I don't know how to do that with the current implementation. https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py (right): https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:20: for k, v in sizes.iteritems()}} nit: indent is wrong https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:88: def testProcessMemoryDump_mmaps(self): Do not change this test. Add a new (simpler one), if you want to test the behavior of adding allocators and mmaps on different events. https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/trace_event_importer.py (right): https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/trace_event_importer.py:205: event['ts'])) Will all events have the same event['ts'] ? Does it matter which one we pick? https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py (right): https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py:1088: def testImportComposableMemoryDumpEvents(self): Is this testing anything not already tested by memory_dump_event_unittest.py ?
+1 to perezju's comments. Please address them before contacting an owner. Thanks, Petr
I made suggested changes, perezju could you take another look? Thanks https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/memory_dump_event.py (right): https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:141: timing correlated with that of other events in the model. On 2016/01/08 16:19:30, perezju wrote: > I'm not all that sold on the idea of fragmenting the constructor into: construct > + AddRawEvent's + ProcessAllocatorDumps. I think that a better solution could be > for the caller to do the grouping of events beforehand and then call the > constructor as ProcessMemoryDumpEvent(process, > list_of_raw_events_for_this_process). > > If you decide to stick with the fragmented constructor, that would be OK but: > - add to this docstring an example of how to create and populate this object. > - make sure to complain if calling other methods before ProcessAllocatorDumps. > - make sure to complain if calling AddRawEvent *after* ProcessAllocatorDumps. > - make sure to complain if ProcessAllocatorDumps is called but no raw events > were added. > - add unittests to check for these conditions. I think this update is not necessary anymore. I don't feel the need to update doc string for this change. https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:186: """ On 2016/01/08 16:19:30, perezju wrote: > This method should complain if you've already called ProcessAllocatorDumps. Acknowledged. https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:187: assert event['ph'] == 'v' and self.process.pid == event['pid'] On 2016/01/08 16:19:30, perezju wrote: > should also check that self.dump_id and event['id'] match. Done. https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:198: self.has_mmaps = True On 2016/01/08 16:19:30, perezju wrote: > Move setting the value of has_mmaps to the processing method below. > > Also (at least from the old implementation) it should be False if the > 'vm_regions' key is in the dictionary but has an empty value. (Or maybe that > never happens?) Done. https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:205: Parse and aggregate the allocator dumps values and the vm regions so that On 2016/01/08 16:19:30, perezju wrote: > If this processes both allocator dumps *and* vm regions, then the name of the > method is misleading. > > Maybe just "ProcessRawEvents()" or ...? Done. https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:291: usage[key] = result On 2016/01/08 16:19:31, perezju wrote: > Hmm.. I'm not super happy about this. We should have a way to distinguish > between that metric not being available (so it's not returned in the results), > from the metric being available *and* having a value of 0. > > Although I don't know how to do that with the current implementation. Yes. But none of the metric can be 0 for the process unless mmaps don't event exist. The metrics pss, java heap, native heap and private dirty are all either non-existent or positive values. Only case is ashmem, but we would report other metrics like discardable as 0 if ashmem is 0. But I can't think of any case in chromium where ashmem can be 0. That is why I made such a change. https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py (right): https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:20: for k, v in sizes.iteritems()}} On 2016/01/08 16:19:31, perezju wrote: > nit: indent is wrong Done. https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:58: memory_dump.AddRawEvent(MakeRawMemoryDumpDict(dump_id, process.pid, start, On 2016/01/08 16:15:57, petrcermak wrote: > nit: In Chromium Python, one of the following two indentation styles should be > used: > > Option A - break immediately after opening bracket and indent 4 spaces > MakeRawMemoryDumpDict( > dump_id, process.pid, start, allocators={ > ... > > Option B - align vertically after the opening bracket: > MakeRawMemoryDumpDict(dump_id, process.pid, start, > allocators={ > ... > > Option A is what you want here (so that the allocators wouldn't be indented too > much). Done. https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:67: allocators={ On 2016/01/08 16:15:57, petrcermak wrote: > ditto Done Thanks https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:78: 'skia': {'allocated_objects_size': 5, On 2016/01/08 16:15:57, petrcermak wrote: > I would personally either write the objects on a single line: > > 'skia': {'allocated_objects_size': 5, ...}, > 'v8': {...}, > > or vertically expand and, consequently, align them: > > 'skia': { > 'allocated_objects_size': 5, > ... > }, > 'v8': { > ... > } Done. https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:88: def testProcessMemoryDump_mmaps(self): On 2016/01/08 16:19:31, perezju wrote: > Do not change this test. Add a new (simpler one), if you want to test the > behavior of adding allocators and mmaps on different events. Done. https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/trace_event_importer.py (right): https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/trace_event_importer.py:205: event['ts'])) On 2016/01/08 16:19:31, perezju wrote: > Will all events have the same event['ts'] ? Does it matter which one we pick? updated with duration https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py (right): https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py:1088: def testImportComposableMemoryDumpEvents(self): On 2016/01/08 16:19:31, perezju wrote: > Is this testing anything not already tested by memory_dump_event_unittest.py ? It was meant to test that the processDumpEvents function calls the constructor functions in order. I guess this is still needed to check if the method composes the correct dumps together or mixes random dumps. https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py:1090: {'name': 'a', 'cat': 'b', 'ph': 'v', 'pid': 52, 'ts': 123, 'id': '123', On 2016/01/08 16:15:57, petrcermak wrote: > This looks much better than before, but I would personally visually expand each > completely (for the sake of readability and ease of modification) > > events = [ > { > 'name': 'a', > 'cat': 'b', > ... > 'args': { > 'dumps': { > 'allocators': { > ... Done.
Thanks for the changes. The interface to build the objects looks better now. Just a few more comments. https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/memory_dump_event.py (right): https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:291: usage[key] = result On 2016/01/11 14:29:00, ssid wrote: > On 2016/01/08 16:19:31, perezju wrote: > > Hmm.. I'm not super happy about this. We should have a way to distinguish > > between that metric not being available (so it's not returned in the results), > > from the metric being available *and* having a value of 0. > > > > Although I don't know how to do that with the current implementation. > > Yes. But none of the metric can be 0 for the process unless mmaps don't event > exist. The metrics pss, java heap, native heap and private dirty are all either > non-existent or positive values. Only case is ashmem, but we would report other > metrics like discardable as 0 if ashmem is 0. But I can't think of any case in > chromium where ashmem can be 0. That is why I made such a change. I'm still not sold on this. Also this is not strictly speaking necessary for the main intent of this CL, so I would prefer to leave this change out and address it on a followup CL. https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/trace_event_importer.py (right): https://codereview.chromium.org/1553183002/diff/90001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/trace_event_importer.py:205: event['ts'])) On 2016/01/11 14:29:01, ssid wrote: > On 2016/01/08 16:19:31, perezju wrote: > > Will all events have the same event['ts'] ? Does it matter which one we pick? > > updated with duration Thanks! Much better now. https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... File tools/telemetry/telemetry/timeline/memory_dump_event.py (right): https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event.py:141: timing correlated with that of other events in the model. Add an "Args" section to document the arguments of the constructor. https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event.py:164: self.dump_id == event['id']) nit: I think that |event| and |self| should be aligned here. https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event.py:172: raw_vm_regions += vm_regions This bit of code seems a bit odd, having this vm_regions variable to momentarily hold the value before the assert. But honestly can't think of a much better way to do this. Maybe just call the variable something more generic like "value" to suggest this is just a temporary variable not used elsewhere in the method. Also, since you add to the list only once, maybe initialize raw_vm_regions to None; and when (if) you find a value then just set it as is (instead of appending to the empty list)? https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event.py:180: raw_allocator_dumps.iteritems()): nit: why not keep the old names (allocators_dict and vm_regions)? / feel free to ignore https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event.py:313: return max(dump.end for dump in self._process_dumps) Add a test that _would_ have failed if you were forget making this change. https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... File tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py (right): https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:11: def MakeRawMemoryDumpDict(dump_id='123456ABCDEF', pid=1234, start=0, mmaps=None, nit: rename to MakeRawMemoryDumpEvent https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:21: for k, v in sizes.iteritems()}} Still looks wrong. Maybe a problem of tabs/spaces? Should be: return {'attrs': {k: {'value': hex(v), 'units': 'bytes'} for k, v in sizes.iteritems()}} https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:51: events = [(MakeRawMemoryDumpDict( This is somewhat hard to parse. Define both events as a single list (not a singleton + append), and add line-breaks/indents if needed to make the structure more visually clear. For example: events = [ MakeRawMemoryDumpDict( pid=process.pid, allocators={ ... } ), MakeRawMemoryDumpDict( pid=process.pid, allocators={ ... } ) ] https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:113: self.assertEquals(value, nit: If you indent the next line on +4 spaces, there should be no args after the '('; i.e. also move "value" one line below. https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:127: EXPECTED_ALLOCATORS = {'v8': {'size': 10}} this is identical to |allocators| above, you can reuse the variable. https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:140: Add a test to make sure that timestamps (start, duration, etc.) on a composed process dump are also correct. https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... File tools/telemetry/telemetry/timeline/trace_event_importer.py (right): https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/trace_event_importer.py:202: dump_events = global_dumps[event['id']].setdefault(process, []) nit: maybe you could also do global_dumps.setdefault(event['id'], {}).setdefault(process, []), to avoid having to create a defaultdict? / feel free to ignore. https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... File tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py (right): https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py:1088: def testImportComposableMemoryDumpEvents(self): I still feel this is more complicated than it has too. Composability should be tested in memory_dump_event_unittest.py. Here you only want to test that raw events are sorted in the correct dump/process id bucket. I would add no allocators/mmaps to these events, instead add *more* events with some matching an non-matching dump/process ids; and then check that you get the expected number of global dumps with correct timestamps/durations.
Made changes, PTAL, Thanks. https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... File tools/telemetry/telemetry/timeline/memory_dump_event.py (right): https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event.py:141: timing correlated with that of other events in the model. On 2016/01/12 15:53:37, perezju wrote: > Add an "Args" section to document the arguments of the constructor. Done. https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event.py:164: self.dump_id == event['id']) On 2016/01/12 15:53:37, perezju wrote: > nit: I think that |event| and |self| should be aligned here. Done. https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event.py:172: raw_vm_regions += vm_regions On 2016/01/12 15:53:37, perezju wrote: > This bit of code seems a bit odd, having this vm_regions variable to momentarily > hold the value before the assert. But honestly can't think of a much better way > to do this. > > Maybe just call the variable something more generic like "value" to suggest this > is just a temporary variable not used elsewhere in the method. > Done. > Also, since you add to the list only once, maybe initialize raw_vm_regions to > None; and when (if) you find a value then just set it as is (instead of > appending to the empty list)? This would need a if condition when i use it. I used = to replace the empty list. https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event.py:180: raw_allocator_dumps.iteritems()): On 2016/01/12 15:53:37, perezju wrote: > nit: why not keep the old names (allocators_dict and vm_regions)? / feel free to > ignore Done. https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event.py:313: return max(dump.end for dump in self._process_dumps) On 2016/01/12 15:53:37, perezju wrote: > Add a test that _would_ have failed if you were forget making this change. Done. https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... File tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py (right): https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:11: def MakeRawMemoryDumpDict(dump_id='123456ABCDEF', pid=1234, start=0, mmaps=None, On 2016/01/12 15:53:38, perezju wrote: > nit: rename to MakeRawMemoryDumpEvent Done. https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:21: for k, v in sizes.iteritems()}} On 2016/01/12 15:53:38, perezju wrote: > Still looks wrong. Maybe a problem of tabs/spaces? Should be: > > return {'attrs': {k: {'value': hex(v), 'units': 'bytes'} > for k, v in sizes.iteritems()}} I thought I will leave it to previous state. fixed it. https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:51: events = [(MakeRawMemoryDumpDict( On 2016/01/12 15:53:37, perezju wrote: > This is somewhat hard to parse. Define both events as a single list (not a > singleton + append), and add line-breaks/indents if needed to make the structure > more visually clear. For example: > > events = [ > MakeRawMemoryDumpDict( > pid=process.pid, > allocators={ > ... > } > ), > MakeRawMemoryDumpDict( > pid=process.pid, > allocators={ > ... > } > ) > ] Done. https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:113: self.assertEquals(value, On 2016/01/12 15:53:37, perezju wrote: > nit: If you indent the next line on +4 spaces, there should be no args after the > '('; i.e. also move "value" one line below. Done. https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:127: EXPECTED_ALLOCATORS = {'v8': {'size': 10}} On 2016/01/12 15:53:38, perezju wrote: > this is identical to |allocators| above, you can reuse the variable. Done. https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:140: On 2016/01/12 15:53:38, perezju wrote: > Add a test to make sure that timestamps (start, duration, etc.) on a composed > process dump are also correct. Hm I changed the existing testDumpEventsTiming. This was not really different from adding another test here. https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... File tools/telemetry/telemetry/timeline/trace_event_importer.py (right): https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/trace_event_importer.py:202: dump_events = global_dumps[event['id']].setdefault(process, []) On 2016/01/12 15:53:38, perezju wrote: > nit: maybe you could also do global_dumps.setdefault(event['id'], > {}).setdefault(process, []), to avoid having to create a defaultdict? / feel > free to ignore. Done. https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... File tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py (right): https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py:1088: def testImportComposableMemoryDumpEvents(self): On 2016/01/12 15:53:38, perezju wrote: > I still feel this is more complicated than it has too. Composability should be > tested in memory_dump_event_unittest.py. Here you only want to test that raw > events are sorted in the correct dump/process id bucket. > > I would add no allocators/mmaps to these events, instead add *more* events with > some matching an non-matching dump/process ids; and then check that you get the > expected number of global dumps with correct timestamps/durations. True. I have added to the previous test and removed this one.
Sweet, looking really good now. Probably time to seek for telemetry owners to review. Ned could you review or suggest someone to? https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... File tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py (right): https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py:1088: def testImportComposableMemoryDumpEvents(self): On 2016/01/12 17:13:18, ssid wrote: > On 2016/01/12 15:53:38, perezju wrote: > > I still feel this is more complicated than it has too. Composability should be > > tested in memory_dump_event_unittest.py. Here you only want to test that raw > > events are sorted in the correct dump/process id bucket. > > > > I would add no allocators/mmaps to these events, instead add *more* events > with > > some matching an non-matching dump/process ids; and then check that you get > the > > expected number of global dumps with correct timestamps/durations. > > True. I have added to the previous test and removed this one. sgtm https://codereview.chromium.org/1553183002/diff/170001/tools/telemetry/teleme... File tools/telemetry/telemetry/timeline/memory_dump_event.py (right): https://codereview.chromium.org/1553183002/diff/170001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event.py:144: process: The process associated with the memory dump. nit: process -> Process object https://codereview.chromium.org/1553183002/diff/170001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event.py:145: dump_events: A list of dump events of the process with the same dump id. nit: add: "and a matching process id." https://codereview.chromium.org/1553183002/diff/170001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event.py:279: usage[key] = result I'm still insisting on removing this change from this CL. Let's address that on a follow up. https://codereview.chromium.org/1553183002/diff/170001/tools/telemetry/teleme... File tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py (right): https://codereview.chromium.org/1553183002/diff/170001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:182: memory_dump_event.ProcessMemoryDumpEvent(process, composable_events), keep the composed process_dump on a variable, and assert that it also has the expected start/end/duration. https://codereview.chromium.org/1553183002/diff/170001/tools/telemetry/teleme... File tools/telemetry/telemetry/timeline/trace_event_importer.py (right): https://codereview.chromium.org/1553183002/diff/170001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/trace_event_importer.py:201: process = self._GetOrCreateProcess(event['pid']) just being slightly paranoid here, but maybe use the event['pid'] itself as key to group into buckets; and then, in the following for-loop, create the Process object required to build the memory dump (that should also be slightly more efficient).
On 2016/01/13 16:30:34, perezju wrote: > Sweet, looking really good now. Probably time to seek for telemetry owners to > review. > > Ned could you review or suggest someone to? > > https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... > File tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py > (right): > > https://codereview.chromium.org/1553183002/diff/150001/tools/telemetry/teleme... > tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py:1088: def > testImportComposableMemoryDumpEvents(self): > On 2016/01/12 17:13:18, ssid wrote: > > On 2016/01/12 15:53:38, perezju wrote: > > > I still feel this is more complicated than it has too. Composability should > be > > > tested in memory_dump_event_unittest.py. Here you only want to test that raw > > > events are sorted in the correct dump/process id bucket. > > > > > > I would add no allocators/mmaps to these events, instead add *more* events > > with > > > some matching an non-matching dump/process ids; and then check that you get > > the > > > expected number of global dumps with correct timestamps/durations. > > > > True. I have added to the previous test and removed this one. > > sgtm > > https://codereview.chromium.org/1553183002/diff/170001/tools/telemetry/teleme... > File tools/telemetry/telemetry/timeline/memory_dump_event.py (right): > > https://codereview.chromium.org/1553183002/diff/170001/tools/telemetry/teleme... > tools/telemetry/telemetry/timeline/memory_dump_event.py:144: process: The > process associated with the memory dump. > nit: process -> Process object > > https://codereview.chromium.org/1553183002/diff/170001/tools/telemetry/teleme... > tools/telemetry/telemetry/timeline/memory_dump_event.py:145: dump_events: A list > of dump events of the process with the same dump id. > nit: add: "and a matching process id." > > https://codereview.chromium.org/1553183002/diff/170001/tools/telemetry/teleme... > tools/telemetry/telemetry/timeline/memory_dump_event.py:279: usage[key] = result > I'm still insisting on removing this change from this CL. Let's address that on > a follow up. > > https://codereview.chromium.org/1553183002/diff/170001/tools/telemetry/teleme... > File tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py (right): > > https://codereview.chromium.org/1553183002/diff/170001/tools/telemetry/teleme... > tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:182: > memory_dump_event.ProcessMemoryDumpEvent(process, composable_events), > keep the composed process_dump on a variable, and assert that it also has the > expected start/end/duration. > > https://codereview.chromium.org/1553183002/diff/170001/tools/telemetry/teleme... > File tools/telemetry/telemetry/timeline/trace_event_importer.py (right): > > https://codereview.chromium.org/1553183002/diff/170001/tools/telemetry/teleme... > tools/telemetry/telemetry/timeline/trace_event_importer.py:201: process = > self._GetOrCreateProcess(event['pid']) > just being slightly paranoid here, but maybe use the event['pid'] itself as key > to group into buckets; and then, in the following for-loop, create the Process > object required to build the memory dump (that should also be slightly more > efficient). lgtm
Thanks. made suggested changes. https://codereview.chromium.org/1553183002/diff/170001/tools/telemetry/teleme... File tools/telemetry/telemetry/timeline/memory_dump_event.py (right): https://codereview.chromium.org/1553183002/diff/170001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event.py:144: process: The process associated with the memory dump. On 2016/01/13 16:30:34, perezju wrote: > nit: process -> Process object Done. https://codereview.chromium.org/1553183002/diff/170001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event.py:145: dump_events: A list of dump events of the process with the same dump id. On 2016/01/13 16:30:34, perezju wrote: > nit: add: "and a matching process id." The comment already says "of the process". https://codereview.chromium.org/1553183002/diff/170001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event.py:279: usage[key] = result On 2016/01/13 16:30:34, perezju wrote: > I'm still insisting on removing this change from this CL. Let's address that on > a follow up. Done. https://codereview.chromium.org/1553183002/diff/170001/tools/telemetry/teleme... File tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py (right): https://codereview.chromium.org/1553183002/diff/170001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:182: memory_dump_event.ProcessMemoryDumpEvent(process, composable_events), On 2016/01/13 16:30:34, perezju wrote: > keep the composed process_dump on a variable, and assert that it also has the > expected start/end/duration. Done. https://codereview.chromium.org/1553183002/diff/170001/tools/telemetry/teleme... File tools/telemetry/telemetry/timeline/trace_event_importer.py (right): https://codereview.chromium.org/1553183002/diff/170001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/trace_event_importer.py:201: process = self._GetOrCreateProcess(event['pid']) On 2016/01/13 16:30:34, perezju wrote: > just being slightly paranoid here, but maybe use the event['pid'] itself as key > to group into buckets; and then, in the following for-loop, create the Process > object required to build the memory dump (that should also be slightly more > efficient). Done.
lgtm thanks! https://codereview.chromium.org/1553183002/diff/170001/tools/telemetry/teleme... File tools/telemetry/telemetry/timeline/memory_dump_event.py (right): https://codereview.chromium.org/1553183002/diff/170001/tools/telemetry/teleme... tools/telemetry/telemetry/timeline/memory_dump_event.py:145: dump_events: A list of dump events of the process with the same dump id. On 2016/01/14 14:26:21, ssid wrote: > On 2016/01/13 16:30:34, perezju wrote: > > nit: add: "and a matching process id." > > The comment already says "of the process". Acknowledged.
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/1553183002/#ps190001 (title: "Fixes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1553183002/190001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1553183002/190001
Message was sent while issue was closed.
Description was changed from ========== [telemetry] Add support for composable process dumps in memory-infra The memory-infra process dumps can be split across multiple process memory dump events in a trace. Telemetry should support merging of them. BUG=461788 ========== to ========== [telemetry] Add support for composable process dumps in memory-infra The memory-infra process dumps can be split across multiple process memory dump events in a trace. Telemetry should support merging of them. BUG=461788 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:190001)
Message was sent while issue was closed.
Description was changed from ========== [telemetry] Add support for composable process dumps in memory-infra The memory-infra process dumps can be split across multiple process memory dump events in a trace. Telemetry should support merging of them. BUG=461788 ========== to ========== [telemetry] Add support for composable process dumps in memory-infra The memory-infra process dumps can be split across multiple process memory dump events in a trace. Telemetry should support merging of them. BUG=461788 Committed: https://crrev.com/e08757506faee7da1bcb7f9b6f8f1d6a8540f038 Cr-Commit-Position: refs/heads/master@{#369428} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/e08757506faee7da1bcb7f9b6f8f1d6a8540f038 Cr-Commit-Position: refs/heads/master@{#369428} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
