Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1196253011:
[Telemetry] Adding Memory Timeline Metric (Closed)
Description[Telemetry] Adding Memory Timeline Metric
Adding a metric that takes memory-infra events from tracing, classifies them
into meaningful aggregates, and surfaces them to be reported as metric results
by telemetry.
BUG=486009
Committed: https://crrev.com/9786cc28c49d9b0637daf1cdcced0aacbc26b93f
Cr-Commit-Position: refs/heads/master@{#337003}
Patch Set 1 #
Total comments: 43
Patch Set 2 : addressed comments from primiano #
Total comments: 12
Patch Set 3 : addressed comments from nednguyen #Patch Set 4 : memory_timeline metric should use IterMemoryDumpEvents #
Total comments: 8
Patch Set 5 : addressed comments from eakuefner #
Total comments: 10
Patch Set 6 : return values for all dumps within interactions #
Total comments: 5
Patch Set 7 : better names for tests #Messages
Total messages: 26 (4 generated)
perezju@chromium.org changed reviewers: + nednguyen@google.com, primiano@chromium.org
@primiano: most of the new code (on, e.g., how to aggregate and classify memory) sits in timeline/memory_dump_event[_unittest].py @nednguyen: please review changes elsewhere, I tried to keep those as small as possible. This CL only adds the metric to telemetry. Another upcoming CL will implement a new benchmark using this metric.
Mostly nits, but the math and the aggregation in memory_dump_event and importer LGTM. Defer to Ned for revieweing the integration with the rest of telemetry, I know very little about that. https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... File tools/telemetry/telemetry/timeline/memory_dump_event.py (right): https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:40: def MatchingChild(self, mapped_file): nit: +Get -> GetMatchingChild https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:45: """ I'd do the early return first, i.e. if not self._children: return None for child... https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:56: MmapCategory('Android', r'^\/dev\/ashmem', [ I think Petr added a negative lookahead here to not swallow the libcmalloc below https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:94: BUCKET_ATTRS = { maybe name this TRACE_ATTRS, so it is clear that they reflect the trace keys (or just add a comment) https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:104: 'overall_pss': '/proportional_resident', Hmm I see what you do here, would be nicer probably if you used a different character to separate the bucket metric from the path, instead of keep using slashes. maybe use a dot and use path.splitext below? i.e. /Android/Ashmem.proportional_resident https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:116: def __repr__(self): Is this for tracing or for the actual telemetry values. If the latter, just make an explicit method "ToTelemetryString" and eventually make __repr__ = ToTelemetryString https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:142: self.start_offset = event['ts'] / 1000.0 nit: add units to this (_ms, _us or whatever it is) https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:153: category = category.MatchingChild(vm_region['mf']) maybe add an extra line here just to make it more readable: mapped_file = vm_region['mf'] category = ...MatchinChild(mapped_file) https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:182: path, name = posixpath.split(path_value) If you use a dot here you can use path.splitext https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:185: def StatsSummary(self): nit: +Get (GetStatsSummary) https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:193: Add one line to explain what events is? https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:201: # all process dump events should have the same dump id nit: add a blnak line before each of these comments, makes the entire function a bit more readable https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:207: assert len(self.process_dumps) == len(all_pids) I like and approve all this paranoy here :) https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:213: duration = self.process_dumps[-1].start_offset Add a comment explaining that duration is the delta between the first and last process dump per each global dump event https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:217: def __repr__(self): ditto (ToTelemetryString) https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... File tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py (right): https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:30: STACK, FILES_APK, DEVICE_GPU) = ALL classy ;-) https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:56: 'proportional_resident')) nit: 4 spaces on indent (you need 2 extra) https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... File tools/telemetry/telemetry/timeline/model.py (right): https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/model.py:77: def IterEventsInThisContainer(self, event_type_predicate, event_predicate): should this be names IterMemoryDumpEventsInThisContainer? https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/w... File tools/telemetry/telemetry/web_perf/metrics/memory_timeline.py (right): https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/w... tools/telemetry/telemetry/web_perf/metrics/memory_timeline.py:22: assert len(interactions) == 1 I think that here you want to just make this a noop if there were no interactions from your test, right?
On 2015/06/23 15:53:24, Primiano Tucci wrote: > Mostly nits, but the math and the aggregation in memory_dump_event and importer > LGTM. > Defer to Ned for revieweing the integration with the rest of telemetry, I know > very little about that. > > https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... > File tools/telemetry/telemetry/timeline/memory_dump_event.py (right): > > https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... > tools/telemetry/telemetry/timeline/memory_dump_event.py:40: def > MatchingChild(self, mapped_file): > nit: +Get -> GetMatchingChild > > https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... > tools/telemetry/telemetry/timeline/memory_dump_event.py:45: """ > I'd do the early return first, i.e. > if not self._children: > return None > for child... > > https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... > tools/telemetry/telemetry/timeline/memory_dump_event.py:56: > MmapCategory('Android', r'^\/dev\/ashmem', [ > I think Petr added a negative lookahead here to not swallow the libcmalloc below > > https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... > tools/telemetry/telemetry/timeline/memory_dump_event.py:94: BUCKET_ATTRS = { > maybe name this TRACE_ATTRS, so it is clear that they reflect the trace keys (or > just add a comment) > > https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... > tools/telemetry/telemetry/timeline/memory_dump_event.py:104: 'overall_pss': > '/proportional_resident', > Hmm I see what you do here, would be nicer probably if you used a different > character to separate the bucket metric from the path, instead of keep using > slashes. > maybe use a dot and use path.splitext below? > i.e. /Android/Ashmem.proportional_resident > > https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... > tools/telemetry/telemetry/timeline/memory_dump_event.py:116: def __repr__(self): > Is this for tracing or for the actual telemetry values. If the latter, just make > an explicit method "ToTelemetryString" and eventually make > __repr__ = ToTelemetryString > > https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... > tools/telemetry/telemetry/timeline/memory_dump_event.py:142: self.start_offset = > event['ts'] / 1000.0 > nit: add units to this (_ms, _us or whatever it is) > > https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... > tools/telemetry/telemetry/timeline/memory_dump_event.py:153: category = > category.MatchingChild(vm_region['mf']) > maybe add an extra line here just to make it more readable: > mapped_file = vm_region['mf'] > category = ...MatchinChild(mapped_file) > > https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... > tools/telemetry/telemetry/timeline/memory_dump_event.py:182: path, name = > posixpath.split(path_value) > If you use a dot here you can use path.splitext > > https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... > tools/telemetry/telemetry/timeline/memory_dump_event.py:185: def > StatsSummary(self): > nit: +Get (GetStatsSummary) > > https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... > tools/telemetry/telemetry/timeline/memory_dump_event.py:193: > Add one line to explain what events is? > > https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... > tools/telemetry/telemetry/timeline/memory_dump_event.py:201: # all process dump > events should have the same dump id > nit: add a blnak line before each of these comments, makes the entire function a > bit more readable > > https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... > tools/telemetry/telemetry/timeline/memory_dump_event.py:207: assert > len(self.process_dumps) == len(all_pids) > I like and approve all this paranoy here :) > > https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... > tools/telemetry/telemetry/timeline/memory_dump_event.py:213: duration = > self.process_dumps[-1].start_offset > Add a comment explaining that duration is the delta between the first and last > process dump per each global dump event > > https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... > tools/telemetry/telemetry/timeline/memory_dump_event.py:217: def __repr__(self): > ditto (ToTelemetryString) > > https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... > File tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py (right): > > https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... > tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:30: STACK, > FILES_APK, DEVICE_GPU) = ALL > classy ;-) > > https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... > tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:56: > 'proportional_resident')) > nit: 4 spaces on indent (you need 2 extra) > > https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... > File tools/telemetry/telemetry/timeline/model.py (right): > > https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... > tools/telemetry/telemetry/timeline/model.py:77: def > IterEventsInThisContainer(self, event_type_predicate, event_predicate): > should this be names IterMemoryDumpEventsInThisContainer? > > https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/w... > File tools/telemetry/telemetry/web_perf/metrics/memory_timeline.py (right): > > https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/w... > tools/telemetry/telemetry/web_perf/metrics/memory_timeline.py:22: assert > len(interactions) == 1 > I think that here you want to just make this a noop if there were no > interactions from your test, right? Before I start to review this, can you show some results where this new metric/benchmark helps catch a regression? i.e: something like intentionally create a memory-related regression and run the benchmark with/without the change.
Ned, I've run a benchmark (similar to the one in 1184393002) using the metric implemented in this CL with: ./tools/perf/run_benchmark run memory.memory_health --browser android-chromium --output-format html Before and after introducing an artificial java leak. You can see the results, and the regression being caught, at: https://x20web.corp.google.com/~perezju/memory_infra/telemetry_results.html (click on "memory" to see). https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... File tools/telemetry/telemetry/timeline/memory_dump_event.py (right): https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:40: def MatchingChild(self, mapped_file): On 2015/06/23 15:53:23, Primiano Tucci wrote: > nit: +Get -> GetMatchingChild Done. https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:45: """ On 2015/06/23 15:53:23, Primiano Tucci wrote: > I'd do the early return first, i.e. > if not self._children: > return None > for child... Done. https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:56: MmapCategory('Android', r'^\/dev\/ashmem', [ On 2015/06/23 15:53:23, Primiano Tucci wrote: > I think Petr added a negative lookahead here to not swallow the libcmalloc below Done. Applied same fix as in https://codereview.appspot.com/246390043/ https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:94: BUCKET_ATTRS = { On 2015/06/23 15:53:22, Primiano Tucci wrote: > maybe name this TRACE_ATTRS, so it is clear that they reflect the trace keys (or > just add a comment) Added a comment. https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:104: 'overall_pss': '/proportional_resident', On 2015/06/23 15:53:23, Primiano Tucci wrote: > Hmm I see what you do here, would be nicer probably if you used a different > character to separate the bucket metric from the path, instead of keep using > slashes. > maybe use a dot and use path.splitext below? > i.e. /Android/Ashmem.proportional_resident Done. https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:116: def __repr__(self): On 2015/06/23 15:53:23, Primiano Tucci wrote: > Is this for tracing or for the actual telemetry values. If the latter, just make > an explicit method "ToTelemetryString" and eventually make > __repr__ = ToTelemetryString For neither of those. It's more for debugging, I like defining the __repr__ method so that we get something reasonably useful when trying to log or print the object. https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:142: self.start_offset = event['ts'] / 1000.0 On 2015/06/23 15:53:23, Primiano Tucci wrote: > nit: add units to this (_ms, _us or whatever it is) Done. https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:153: category = category.MatchingChild(vm_region['mf']) On 2015/06/23 15:53:23, Primiano Tucci wrote: > maybe add an extra line here just to make it more readable: > mapped_file = vm_region['mf'] > category = ...MatchinChild(mapped_file) Done. https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:182: path, name = posixpath.split(path_value) On 2015/06/23 15:53:22, Primiano Tucci wrote: > If you use a dot here you can use path.splitext using just rsplit on a '.', since splitext get's confused with things like '/.foo' (It would think the whole file name is ".foo" with no extension.) https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:185: def StatsSummary(self): On 2015/06/23 15:53:23, Primiano Tucci wrote: > nit: +Get (GetStatsSummary) Done. https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:193: On 2015/06/23 15:53:23, Primiano Tucci wrote: > Add one line to explain what events is? Done. https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:201: # all process dump events should have the same dump id On 2015/06/23 15:53:23, Primiano Tucci wrote: > nit: add a blnak line before each of these comments, makes the entire function a > bit more readable Done. https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:207: assert len(self.process_dumps) == len(all_pids) On 2015/06/23 15:53:23, Primiano Tucci wrote: > I like and approve all this paranoy here :) :) https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:213: duration = self.process_dumps[-1].start_offset On 2015/06/23 15:53:22, Primiano Tucci wrote: > Add a comment explaining that duration is the delta between the first and last > process dump per each global dump event Done. https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event.py:217: def __repr__(self): On 2015/06/23 15:53:23, Primiano Tucci wrote: > ditto (ToTelemetryString) same as above https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... File tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py (right): https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/memory_dump_event_unittest.py:56: 'proportional_resident')) On 2015/06/23 15:53:23, Primiano Tucci wrote: > nit: 4 spaces on indent (you need 2 extra) Done. https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... File tools/telemetry/telemetry/timeline/model.py (right): https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/model.py:77: def IterEventsInThisContainer(self, event_type_predicate, event_predicate): On 2015/06/23 15:53:24, Primiano Tucci wrote: > should this be names IterMemoryDumpEventsInThisContainer? I forgot to add a comment for review. This is overriding the IterEventsInThisContainer from the parent class (which yields nothing). This is only needed so that our MemoryDumpEvent's appear when doing ShiftWorldToZero. See my other comment below ... btw, ned, maybe you have a better suggestion on how to do this? https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/model.py:190: event.start -= shift_amount ... we need our MemoryDumpEvent's to show up here so that timestamps between page interactions and memory dumps correlate. https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... File tools/telemetry/telemetry/timeline/trace_event_importer.py (right): https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/trace_event_importer.py:187: return # skip events without vm_regions I'm not skipping these any more, so people wanting to extend this CL to gather other memory-infra tracing metrics (1) don't see their events disappearing and (2) don't need to make any further changes at all on this file. https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/w... File tools/telemetry/telemetry/web_perf/metrics/memory_timeline.py (right): https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/w... tools/telemetry/telemetry/web_perf/metrics/memory_timeline.py:22: assert len(interactions) == 1 On 2015/06/23 15:53:24, Primiano Tucci wrote: > I think that here you want to just make this a noop if there were no > interactions from your test, right? Done. I made it work with any number of interactions. We still report only one result per page (the last one contained within all supplied interactions). Note that for tests not enabling memory-infra tracing, they'll just get None's. https://codereview.chromium.org/1196253011/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/memory_dump_event.py (right): https://codereview.chromium.org/1196253011/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:141: GetStatsSummary will report all zeros. I added this since I'm not skipping dumps without mmaps any more. https://codereview.chromium.org/1196253011/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:230: self.has_mmaps = has_mmaps.pop() @primiano, I'm assuming either all dumps with the same id have mmaps, or none of them have. Is that correct? https://codereview.chromium.org/1196253011/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py (right): https://codereview.chromium.org/1196253011/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py:1024: 'id': '1234ABCD'}, no need to fake mmaps any more. https://codereview.chromium.org/1196253011/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/memory_timeline.py (right): https://codereview.chromium.org/1196253011/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/memory_timeline.py:44: name='memory_infra_' + name, Adding this so benchmarks can quickly find and filter results that are reported by this metric.
nednguyen@google.com changed reviewers: + eakuefner@chromium.org
https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... File tools/telemetry/telemetry/timeline/model.py (right): https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/model.py:77: def IterEventsInThisContainer(self, event_type_predicate, event_predicate): On 2015/06/24 15:35:35, perezju wrote: > On 2015/06/23 15:53:24, Primiano Tucci wrote: > > should this be names IterMemoryDumpEventsInThisContainer? > > I forgot to add a comment for review. This is overriding the > IterEventsInThisContainer from the parent class (which yields nothing). This is > only needed so that our MemoryDumpEvent's appear when doing ShiftWorldToZero. > See my other comment below ... > > btw, ned, maybe you have a better suggestion on how to do this? this should return self.itertools.chain(iter(self.flow_events), iter(self.memory_dump_events)) Callsite needs to use the appropriate event_type_predicate / event_predicate to get only memory dump events. https://codereview.chromium.org/1196253011/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/memory_timeline_unittest.py (right): https://codereview.chromium.org/1196253011/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/memory_timeline_unittest.py:39: class MemoryTimelineMetricUnitTest(unittest.TestCase): Can you add test case with multiple interactions?
https://codereview.chromium.org/1196253011/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/memory_dump_event.py (right): https://codereview.chromium.org/1196253011/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:214: def __init__(self, events): Update documentation to says that these events are memory dump events. https://codereview.chromium.org/1196253011/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:215: assert events Also add assertion that all events have phase 'v' https://codereview.chromium.org/1196253011/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/model.py (right): https://codereview.chromium.org/1196253011/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/model.py:73: self.memory_dump_events = [] The fact that the other properties are not immutable is a mistake. Please make this field immutable by: self._memory_dump_events = None def SetMemoryDumpEvents(self, memory_dump_events): assert not self._frozen and memory_dump_events is None self._memory_dump_events = tuple(sort(memory_dump_events, key=lambda event: event.start))) @property def memory_dump_events(self): return self._memory_dump_events
https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... File tools/telemetry/telemetry/timeline/model.py (right): https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/model.py:77: def IterEventsInThisContainer(self, event_type_predicate, event_predicate): On 2015/06/25 02:05:37, nednguyen wrote: > On 2015/06/24 15:35:35, perezju wrote: > > On 2015/06/23 15:53:24, Primiano Tucci wrote: > > > should this be names IterMemoryDumpEventsInThisContainer? > > > > I forgot to add a comment for review. This is overriding the > > IterEventsInThisContainer from the parent class (which yields nothing). This > is > > only needed so that our MemoryDumpEvent's appear when doing ShiftWorldToZero. > > See my other comment below ... > > > > btw, ned, maybe you have a better suggestion on how to do this? > > this should return self.itertools.chain(iter(self.flow_events), > iter(self.memory_dump_events)) > > Callsite needs to use the appropriate event_type_predicate / event_predicate to > get only memory dump events. I'm a bit confused. Are you saying that this method should ignore the event_type_predicate and event_predicate arguments? I thought that the filtering was supposed to happen here, using the functions supplied by the caller. Also, it seems that I shouldn't yield flow events here, otherwise they end up being double-shifted during ShiftWorldToZero, because these are *also* contained in and yielded by thread objects (c.f. https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... and https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...). Actually, I don't really understand why there is a model.flow_events, since the list is populated, but nothing (other than unittests) seems to read from it. https://codereview.chromium.org/1196253011/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/memory_dump_event.py (right): https://codereview.chromium.org/1196253011/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:214: def __init__(self, events): On 2015/06/25 02:19:12, nednguyen wrote: > Update documentation to says that these events are memory dump events. Done. https://codereview.chromium.org/1196253011/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:215: assert events On 2015/06/25 02:19:12, nednguyen wrote: > Also add assertion that all events have phase 'v' Done. Added assertion on the ProcessMemoryDump constructor. https://codereview.chromium.org/1196253011/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/model.py (right): https://codereview.chromium.org/1196253011/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/model.py:73: self.memory_dump_events = [] On 2015/06/25 02:19:12, nednguyen wrote: > The fact that the other properties are not immutable is a mistake. Please make > this field immutable by: > > self._memory_dump_events = None > > > def SetMemoryDumpEvents(self, memory_dump_events): > assert not self._frozen and memory_dump_events is None > self._memory_dump_events = tuple(sort(memory_dump_events, key=lambda event: > event.start))) > > @property > def memory_dump_events(self): > return self._memory_dump_events Done. I went for an iterator, though, rather than exposing the tuple as a property. https://codereview.chromium.org/1196253011/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/memory_timeline_unittest.py (right): https://codereview.chromium.org/1196253011/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/memory_timeline_unittest.py:39: class MemoryTimelineMetricUnitTest(unittest.TestCase): On 2015/06/25 02:05:37, nednguyen wrote: > Can you add test case with multiple interactions? Done.
Style nit throughout: for docstrings where you have to continue a line on the next line, indent by four spaces rather than two, as if you were calling a function. https://codereview.chromium.org/1196253011/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/trace_event_importer.py (right): https://codereview.chromium.org/1196253011/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/trace_event_importer.py:29: self._all_memory_dump_events = {} nit: self._all_memory_dump_events_by_dump_id would make it clearer that this is a dict, since the rest of the _all_* fields are lists. https://codereview.chromium.org/1196253011/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/trace_event_importer.py:187: 'Memroy dump event with missing dump id.') nit: Memroy -> Memory https://codereview.chromium.org/1196253011/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/memory_timeline_unittest.py (right): https://codereview.chromium.org/1196253011/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/memory_timeline_unittest.py:13: class MockMemoryDump(object): Try using the mock library instead of mocking these classes by hand. Check out tab_switching_unittest.py in https://codereview.chromium.org/1152763002 to see how Dean imports the library (since there's a magic incantation), and then uses it to mock his classes. (Patchset 4 to see how he was doing it by hand before). https://codereview.chromium.org/1196253011/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/memory_timeline_unittest.py:17: # For simplicity, we tuck the same value on all reported metrics. I'm confused about what "tuck" means here.
https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... File tools/telemetry/telemetry/timeline/model.py (right): https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/model.py:77: def IterEventsInThisContainer(self, event_type_predicate, event_predicate): On 2015/06/25 10:36:47, perezju wrote: > On 2015/06/25 02:05:37, nednguyen wrote: > > On 2015/06/24 15:35:35, perezju wrote: > > > On 2015/06/23 15:53:24, Primiano Tucci wrote: > > > > should this be names IterMemoryDumpEventsInThisContainer? > > > > > > I forgot to add a comment for review. This is overriding the > > > IterEventsInThisContainer from the parent class (which yields nothing). This > > is > > > only needed so that our MemoryDumpEvent's appear when doing > ShiftWorldToZero. > > > See my other comment below ... > > > > > > btw, ned, maybe you have a better suggestion on how to do this? > > > > this should return self.itertools.chain(iter(self.flow_events), > > iter(self.memory_dump_events)) > > > > Callsite needs to use the appropriate event_type_predicate / event_predicate > to > > get only memory dump events. > > I'm a bit confused. Are you saying that this method should ignore the > event_type_predicate and event_predicate arguments? I thought that the filtering > was supposed to happen here, using the functions supplied by the caller. > > Also, it seems that I shouldn't yield flow events here, otherwise they end up > being double-shifted during ShiftWorldToZero, because these are *also* contained > in and yielded by thread objects (c.f. > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > and > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...). > > > Actually, I don't really understand why there is a model.flow_events, since the > list is populated, but nothing (other than unittests) seems to read from it. Yes, this should also use event_type_predicate & event_predicate to filter out events. Also the flow_events here should not be the same as the ones owned by thread_objects. If they are the same, then the flow events here should be removed.
Thanks for the comments. I've addressed them all on my latest patch. https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... File tools/telemetry/telemetry/timeline/model.py (right): https://codereview.chromium.org/1196253011/diff/1/tools/telemetry/telemetry/t... tools/telemetry/telemetry/timeline/model.py:77: def IterEventsInThisContainer(self, event_type_predicate, event_predicate): On 2015/06/25 16:59:30, nednguyen wrote: > On 2015/06/25 10:36:47, perezju wrote: > > On 2015/06/25 02:05:37, nednguyen wrote: > > > On 2015/06/24 15:35:35, perezju wrote: > > > > On 2015/06/23 15:53:24, Primiano Tucci wrote: > > > > > should this be names IterMemoryDumpEventsInThisContainer? > > > > > > > > I forgot to add a comment for review. This is overriding the > > > > IterEventsInThisContainer from the parent class (which yields nothing). > This > > > is > > > > only needed so that our MemoryDumpEvent's appear when doing > > ShiftWorldToZero. > > > > See my other comment below ... > > > > > > > > btw, ned, maybe you have a better suggestion on how to do this? > > > > > > this should return self.itertools.chain(iter(self.flow_events), > > > iter(self.memory_dump_events)) > > > > > > Callsite needs to use the appropriate event_type_predicate / event_predicate > > to > > > get only memory dump events. > > > > I'm a bit confused. Are you saying that this method should ignore the > > event_type_predicate and event_predicate arguments? I thought that the > filtering > > was supposed to happen here, using the functions supplied by the caller. > > > > Also, it seems that I shouldn't yield flow events here, otherwise they end up > > being double-shifted during ShiftWorldToZero, because these are *also* > contained > > in and yielded by thread objects (c.f. > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > and > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...). > > > > > > Actually, I don't really understand why there is a model.flow_events, since > the > > list is populated, but nothing (other than unittests) seems to read from it. > > Yes, this should also use event_type_predicate & event_predicate to filter out > events. > > Also the flow_events here should not be the same as the ones owned by > thread_objects. If they are the same, then the flow events here should be > removed. Agreed. I'm not familiar with flow events, and I don't think I should alter how they work on this CL. So I started crbug.com/504811 get that fixed separately. https://codereview.chromium.org/1196253011/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/trace_event_importer.py (right): https://codereview.chromium.org/1196253011/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/trace_event_importer.py:29: self._all_memory_dump_events = {} On 2015/06/25 15:56:20, eakuefner wrote: > nit: self._all_memory_dump_events_by_dump_id would make it clearer that this is > a dict, since the rest of the _all_* fields are lists. Done. https://codereview.chromium.org/1196253011/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/trace_event_importer.py:187: 'Memroy dump event with missing dump id.') On 2015/06/25 15:56:20, eakuefner wrote: > nit: Memroy -> Memory Done. https://codereview.chromium.org/1196253011/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/memory_timeline_unittest.py (right): https://codereview.chromium.org/1196253011/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/memory_timeline_unittest.py:13: class MockMemoryDump(object): On 2015/06/25 15:56:20, eakuefner wrote: > Try using the mock library instead of mocking these classes by hand. > > Check out tab_switching_unittest.py in > https://codereview.chromium.org/1152763002 to see how Dean imports the library > (since there's a magic incantation), and then uses it to mock his classes. > (Patchset 4 to see how he was doing it by hand before). Done. Thanks for the suggestion, does look much cleaner. https://codereview.chromium.org/1196253011/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/memory_timeline_unittest.py:17: # For simplicity, we tuck the same value on all reported metrics. On 2015/06/25 15:56:20, eakuefner wrote: > I'm confused about what "tuck" means here. I re-phrased this a bit.
Friendly ping. Any more comments on this?
lgtm
https://codereview.chromium.org/1196253011/diff/80001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/memory_dump_event.py (right): https://codereview.chromium.org/1196253011/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:53: Looks like these memory data are specific to android platform. Just curious, does any of this work on other OSes? https://codereview.chromium.org/1196253011/diff/80001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/memory_timeline.py (right): https://codereview.chromium.org/1196253011/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/memory_timeline.py:28: # We report the latest dump that happened during the interactions. Why? Does the last dump's memory stats captures all the memory related metrics of the dumps that happen before it? Another option here is to use list_of_scalar.ListOfScalar to store all metrics of all dumps that happen during interactionrs. https://codereview.chromium.org/1196253011/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/memory_timeline.py:44: logging.warning('No memory dumps found for %s', results.current_page) You don't need this warning. The none_reason should show up in the chartjson results output. https://codereview.chromium.org/1196253011/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/memory_timeline.py:49: name='memory_infra_' + name, Can you simplify this to name='memory_%s' % name? We want the metric names in telemetry to be as generic. 'memory_infra' name is kinda specific to some sort of organizational dependence.
https://codereview.chromium.org/1196253011/diff/80001/tools/telemetry/telemet... File tools/telemetry/telemetry/timeline/memory_dump_event.py (right): https://codereview.chromium.org/1196253011/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/timeline/memory_dump_event.py:53: On 2015/06/30 15:47:31, nednguyen wrote: > Looks like these memory data are specific to android platform. Just curious, > does any of this work on other OSes? For the moment Linux is the only other platform that can push the same data. There is WIP from Win folks to get the same for windows. Right now, on Linux, everything % the "Android" category would be dumped. https://codereview.chromium.org/1196253011/diff/80001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/memory_timeline.py (right): https://codereview.chromium.org/1196253011/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/memory_timeline.py:28: # We report the latest dump that happened during the interactions. On 2015/06/30 15:47:31, nednguyen wrote: > Why? Does the last dump's memory stats captures all the memory related metrics > of the dumps that happen before it? I think that the main goal right now is to start capturing memory in steady state (i.e. after page is loaded), and then evolve as we get the devtools api to finer grained snapshots.
https://codereview.chromium.org/1196253011/diff/80001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/memory_timeline.py (right): https://codereview.chromium.org/1196253011/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/memory_timeline.py:28: # We report the latest dump that happened during the interactions. On 2015/06/30 15:59:36, Primiano Tucci wrote: > On 2015/06/30 15:47:31, nednguyen wrote: > > Why? Does the last dump's memory stats captures all the memory related metrics > > of the dumps that happen before it? > I think that the main goal right now is to start capturing memory in steady > state (i.e. after page is loaded), If we want to capture memory only in steady state, we should be able to adjust the interaction records so that it doesn't capture things before page is loaded. e.g: the page's code can be written like: RunPageInteracions(action_runner): self._WaitUntilThingsSettleDown(action_runner) with action_runner.CreateIntearction('memory_dumps'): action_runner.Wait(3) Interaction record allows us to flexibly define where in the browser events timeline we want to compute the metrics. I would prefer us to exhaust that property, and make the metrics computation give the summarized stats to all related events that happen during the interactions. > and then evolve as we get the devtools api to > finer grained snapshots.
Thanks for the suggestions Ned. PTAL. https://codereview.chromium.org/1196253011/diff/80001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/memory_timeline.py (right): https://codereview.chromium.org/1196253011/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/memory_timeline.py:28: # We report the latest dump that happened during the interactions. On 2015/06/30 16:08:40, nednguyen wrote: > On 2015/06/30 15:59:36, Primiano Tucci wrote: > > On 2015/06/30 15:47:31, nednguyen wrote: > > > Why? Does the last dump's memory stats captures all the memory related > metrics > > > of the dumps that happen before it? > > I think that the main goal right now is to start capturing memory in steady > > state (i.e. after page is loaded), > > If we want to capture memory only in steady state, we should be able to adjust > the interaction records so that it doesn't capture things before page is loaded. > > e.g: the page's code can be written like: > RunPageInteracions(action_runner): > self._WaitUntilThingsSettleDown(action_runner) > with action_runner.CreateIntearction('memory_dumps'): > action_runner.Wait(3) > > > Interaction record allows us to flexibly define where in the browser events > timeline we want to compute the metrics. I would prefer us to exhaust that > property, and make the metrics computation give the summarized stats to all > related events that happen during the interactions. > > > and then evolve as we get the devtools api to > > finer grained snapshots. > Ok, I tried this and the code ended up a bit simpler, so it was probably a good idea. I'll also update CL/1208933003 accordingly. https://codereview.chromium.org/1196253011/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/memory_timeline.py:44: logging.warning('No memory dumps found for %s', results.current_page) On 2015/06/30 15:47:31, nednguyen wrote: > You don't need this warning. The none_reason should show up in the chartjson > results output. Done. https://codereview.chromium.org/1196253011/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/memory_timeline.py:49: name='memory_infra_' + name, On 2015/06/30 15:47:31, nednguyen wrote: > Can you simplify this to name='memory_%s' % name? We want the metric names in > telemetry to be as generic. 'memory_infra' name is kinda specific to some sort > of organizational dependence. Done.
lgtm with nits https://codereview.chromium.org/1196253011/diff/100001/tools/telemetry/teleme... File tools/telemetry/telemetry/web_perf/metrics/memory_timeline_unittest.py (right): https://codereview.chromium.org/1196253011/diff/100001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/memory_timeline_unittest.py:90: def testMultipleInteractions_withDumpInSecond(self): testMultipleInteractionsWithDumpInSecond. What does DumpInSecond mean here? https://codereview.chromium.org/1196253011/diff/100001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/memory_timeline_unittest.py:99: def testMultipleInteractions_withoutDumpInSecond(self): Same question here. https://codereview.chromium.org/1196253011/diff/100001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/memory_timeline_unittest.py:123: Can you add a simple test in which the |stats| part of MockMemoryDump contains different metrics values?
https://codereview.chromium.org/1196253011/diff/100001/tools/telemetry/teleme... File tools/telemetry/telemetry/web_perf/metrics/memory_timeline_unittest.py (right): https://codereview.chromium.org/1196253011/diff/100001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/memory_timeline_unittest.py:90: def testMultipleInteractions_withDumpInSecond(self): On 2015/06/30 17:59:22, nednguyen wrote: > testMultipleInteractionsWithDumpInSecond. > > What does DumpInSecond mean here? I renamed and reordered a bit the tests. Hopefully they make more sense now. https://codereview.chromium.org/1196253011/diff/100001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/memory_timeline_unittest.py:123: On 2015/06/30 17:59:22, nednguyen wrote: > Can you add a simple test in which the |stats| part of MockMemoryDump contains > different metrics values? That one is: testResultsGroupingByMetric
The CQ bit was checked by perezju@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, eakuefner@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/1196253011/#ps120001 (title: "better names for tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1196253011/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/9786cc28c49d9b0637daf1cdcced0aacbc26b93f Cr-Commit-Position: refs/heads/master@{#337003} |
