[Telemetry] Fix idle wakeup reporting in the face of dead processes

[Telemetry] Fix idle wakeup reporting in the face of dead processes

Telemetry does not account for process death when computing deltas of various process statistics over a browser run.

CPU stats are currently stored in a dictionary of POD types aggregated by process type which gives no room for separating dead processes over a run.

Ultimately the goal is to report CPU stats in the timeline on a per-process basis.

Towards this goal and to fix counting of dead processes, this CL adds an IdleStatsData object which inherits from TimelineData. IdleStatsData keeps
track of idle wake ups by PID and is able to omit processes which have died during a run from the final reported result.

BUG=343373
Committed: https://crrev.com/edde24b3815fdbc888c675fb24cb21a25186c989
Cr-Commit-Position: refs/heads/master@{#292422}

[tonyg, 2014-08-24 16:53:44]

This looks like a really positive incremental step towards better timeline
integration. My highest level thought is that at this point we might want to
also consider what at least one other such stat will look like in this model to
help guide the decision about how to name this (see inline for more). I also
think Ned might be interested in taking a review pass.

https://codereview.chromium.org/501813002/diff/1/tools/perf/metrics/power.py
File tools/perf/metrics/power.py (right):

https://codereview.chromium.org/501813002/diff/1/tools/perf/metrics/power.py#...
tools/perf/metrics/power.py:162: def _SubtractCpuStats(cpu_stats,
start_cpu_stats):
I was hoping this whole method would go away from this file in favor of an
inlined subtraction. Is that feasible?

https://codereview.chromium.org/501813002/diff/1/tools/perf/metrics/power.py#...
tools/perf/metrics/power.py:183: # Verify that the IdleWakeupCount field has
been converted to a
Let's just kill this comment. It answers "what" instead of "why" which is
usually extraneous.

https://codereview.chromium.org/501813002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/timeline_objects.py (right):

https://codereview.chromium.org/501813002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/timeline_objects.py:10: class
IdleStatsData(timeline_data.TimelineData):
I think this implementation should live in
tools/telemetry/telemetry/timeline/idle_wakeup_timeline_data.py and be named
IdleWakeupTimelineData. WDYT?

Another thought is that no code in this file other than the naming is actually
specific to idle wakeups. What if we called it something like
ProcessStatisticTimelineData and either passed a string to the ctor indicating
the type or else made trivial subclasses for each time. I mention that because
the dicts returned by Browser.cpu_stats, io_stats, memory_stats have the same
problem.

https://codereview.chromium.org/501813002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/timeline_objects.py:18:
self._idle_wakeup_count_by_pid = {pid : idle_wakeup_count}
nit: no space before :

https://codereview.chromium.org/501813002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/timeline_objects.py:26: the processes
that are still alive at the end of the second measurement."""
What about new processes (those in the second, but not the first)? Should we
include all wakeups for the lifetime of those processes?

I asked b/c this'll be common for navigations which create a new renderer.

[jeremy, 2014-08-25 08:41:00]

On Sun, Aug 24, 2014 at 7:53 PM, <tonyg@chromium.org> wrote:

> https://codereview.chromium.org/501813002/diff/1/tools/
> perf/metrics/power.py
> File tools/perf/metrics/power.py (right):
>
> https://codereview.chromium.org/501813002/diff/1/tools/
> perf/metrics/power.py#newcode162
> tools/perf/metrics/power.py:162: def _SubtractCpuStats(cpu_stats,
> start_cpu_stats):
> I was hoping this whole method would go away from this file in favor of
> an inlined subtraction. Is that feasible?
>

cpu_stats are bucketed by process type, for each process type the content
is an IdleWakeupCount object.  In order to make the subtraction entirely
inline we'd need to change the way that cpu_stats are stored (remove the
process type bucketing or use an object that supports bucketing and direct
subtraction) this would be a larger change than this CL.


> https://codereview.chromium.org/501813002/diff/1/tools/
> telemetry/telemetry/core/platform/timeline_objects.py#newcode26
> tools/telemetry/telemetry/core/platform/timeline_objects.py:26: the
> processes that are still alive at the end of the second measurement."""
> What about new processes (those in the second, but not the first)?
> Should we include all wakeups for the lifetime of those processes?
>
> I asked b/c this'll be common for navigations which create a new
> renderer.
>

Makes sense, we should include new processes and just exclude processes
that have died.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nednguyen, 2014-08-25 15:42:01]

https://codereview.chromium.org/501813002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/timeline_objects.py (right):

https://codereview.chromium.org/501813002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/timeline_objects.py:10: class
IdleStatsData(timeline_data.TimelineData):
I think this patch is using timeline_data interface for a wrong purpose.
Traditionally, it has been used as a thin wrapper for raw data that can be
consumed by different importer method, it doesn't mean to contain helper methods
for processing the specific raw data.

Nat is working on a patch to remove timeline_data interface, and move these data
to TraceValuePart:

https://codereview.chromium.org/441873007/diff/1/tools/telemetry/telemetry/va...

I think the right approach would be: platform gives the raw data, importer
processes raw data and augment them to model.

[jeremy, 2014-08-26 14:14:16]

Ready for another look.

I've looked at other stats and it appears this pattern will work for those as
well.

https://codereview.chromium.org/501813002/diff/1/tools/perf/metrics/power.py
File tools/perf/metrics/power.py (right):

https://codereview.chromium.org/501813002/diff/1/tools/perf/metrics/power.py#...
tools/perf/metrics/power.py:183: # Verify that the IdleWakeupCount field has
been converted to a
On 2014/08/24 16:53:43, tonyg wrote:
> Let's just kill this comment. It answers "what" instead of "why" which is
> usually extraneous.

Done.

https://codereview.chromium.org/501813002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/timeline_objects.py (right):

https://codereview.chromium.org/501813002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/timeline_objects.py:10: class
IdleStatsData(timeline_data.TimelineData):
Done with trivial subclasses, I haven't moved this into timeline pending the
dust settling from the changes Ned mentioned.

https://codereview.chromium.org/501813002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/timeline_objects.py:18:
self._idle_wakeup_count_by_pid = {pid : idle_wakeup_count}
On 2014/08/24 16:53:44, tonyg wrote:
> nit: no space before :

Done.

https://codereview.chromium.org/501813002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/timeline_objects.py:26: the processes
that are still alive at the end of the second measurement."""
On 2014/08/24 16:53:44, tonyg wrote:
> What about new processes (those in the second, but not the first)? Should we
> include all wakeups for the lifetime of those processes?
> 
> I asked b/c this'll be common for navigations which create a new renderer.

Done.

[nednguyen, 2014-08-26 14:56:46]

Would you mind filing a bug to move these platform stats to model and tracing
controller?

https://codereview.chromium.org/501813002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/process_statistic_timeline_data.py
(right):

https://codereview.chromium.org/501813002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/process_statistic_timeline_data.py:46: 
Would it be useful if we keep track of dead processes add some method that
output them?

https://codereview.chromium.org/501813002/diff/20001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/platform/process_statistic_timeline_data_unittest.py
(right):

https://codereview.chromium.org/501813002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/process_statistic_timeline_data_unittest.py:33:

Can you add test for dead processes case?

[jeremy, 2014-08-28 12:56:53]

Fixed, ready for another look.

https://codereview.chromium.org/501813002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/process_statistic_timeline_data.py
(right):

https://codereview.chromium.org/501813002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/process_statistic_timeline_data.py:46: 
IMHO - No, for one I think if this does interest us then it's better handled in
other parts of the code.
Apart from that, I don't think it's really an error for a process to go away
while a test is running (e.g. the utility process doing something while a test
is running).

What is important is to calculate the idle wakeup numbers correctly.

https://codereview.chromium.org/501813002/diff/20001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/platform/process_statistic_timeline_data_unittest.py
(right):

https://codereview.chromium.org/501813002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/process_statistic_timeline_data_unittest.py:33:

Thanks! Good catch!

[nednguyen, 2014-08-28 13:26:13]

https://codereview.chromium.org/501813002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/mac_platform_backend.py (right):

https://codereview.chromium.org/501813002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/mac_platform_backend.py:57: """Return
current cpu processing time of pid in seconds."""
This docstring should be updated.

[jeremy, 2014-08-28 13:58:14]

Review latency between us is about 24 hours for a full cycle, if there's nothing
egregious going on please consider an lgm with comments :)

Thanks!

https://codereview.chromium.org/501813002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/mac_platform_backend.py (right):

https://codereview.chromium.org/501813002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/mac_platform_backend.py:57: """Return
current cpu processing time of pid in seconds."""
Outside the scope of this CL, but changed to match the comment in
Browser.cpu_stats .

[nednguyen, 2014-08-28 15:17:32]

lgtm

[nednguyen, 2014-08-28 15:29:00]

https://codereview.chromium.org/501813002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/process_statistic_timeline_data.py
(right):

https://codereview.chromium.org/501813002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/process_statistic_timeline_data.py:30: k
in my_dict.keys()})
Shouldn't it be an error if some values are less than 0 after the subtraction? 
Also we should have timestamp info with top command, is that right? It would be
nice if __sub__ only work for later - previous. But since we will move all of
these logic to importer, so I am fine with skipping this.

[jeremy, 2014-08-28 16:28:56]

The CQ bit was checked by jeremy@chromium.org

[commit-bot: I haz the power, 2014-08-28 16:30:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jeremy@chromium.org/501813002/60001

[commit-bot: I haz the power, 2014-08-28 18:37:55]

Committed patchset #4 (id:60001) as b19a80f5096dc3974ef9bfc5bdf3b1fe6a3b55cd

[commit-bot: I haz the power, 2014-09-10 03:00:52]

Patchset 4 (id:??) landed as
https://crrev.com/edde24b3815fdbc888c675fb24cb21a25186c989
Cr-Commit-Position: refs/heads/master@{#292422}