telemetry: Fix killing the perf profiler

telemetry: Fix killing the perf profiler

Telemetry was failing to kill the perf profiler on Android because
pylib's KillAll command didn't account for multiple instances of the
same binary being running. The KillAll function would randomly kill one
of the running instances and then time out waiting for the instances to
go away.

This patch changes pylib's KillAll helper send the signal to all running
instances instead of just one. To do that we need to extend the GetPids
function to return all running instances of a binary instead of just one.

BUG=512564
Committed: https://crrev.com/e5c729f794a24ba5195f8e56bd90a8b179097bdb
Cr-Commit-Position: refs/heads/master@{#340496}

[Sami, 2015-07-24 12:33:16]

skyostil@chromium.org changed reviewers:
+ jbudorick@chromium.org, perezju@chromium.org

[Sami, 2015-07-24 12:33:41]

PTAL. Open to other suggestions for fixing this.

[perezju, 2015-07-24 14:03:10]

Good catch! I just have some comments about the implementation.

Also, do you think maybe something like this would be the fix needed for
http://crbug.com/442319 ?

https://codereview.chromium.org/1254843002/diff/1/build/android/pylib/device/...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/1254843002/diff/1/build/android/pylib/device/...
build/android/pylib/device/device_utils.py:1570: multiple_instances=False):
I don't think John will like adding an extra option (maybe he will prove me
wrong?), and it leads to a somewhat messy implementation.

I propose having a GetProcessPidPairs (or something more artfully named), with
essentially the same implementation as the current GetPids, but returning a list
of (binary_name, pid) pairs.

The implementation of GetPids would then just become:

  return dict(self.GetProcessPidPairs(...))

so we don't change the expectations of existing callers, and KillAll can use the
GetProcessPidPairs to really kill all matching processes.

https://codereview.chromium.org/1254843002/diff/1/tools/telemetry/telemetry/i...
File tools/telemetry/telemetry/internal/platform/profiler/perf_profiler.py
(right):

https://codereview.chromium.org/1254843002/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/platform/profiler/perf_profiler.py:111:
device.KillAll(binary_name, signum=signal.SIGINT, blocking=True)
you should be able to pass quiet=True here

[Sami, 2015-07-24 14:36:35]

Thanks!

On 2015/07/24 14:03:10, perezju wrote:
> Good catch! I just have some comments about the implementation.
> 
> Also, do you think maybe something like this would be the fix needed for
> http://crbug.com/442319 ?

Yeah, that sounds like it might be related.

[Sami, 2015-07-24 14:36:41]

https://codereview.chromium.org/1254843002/diff/1/build/android/pylib/device/...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/1254843002/diff/1/build/android/pylib/device/...
build/android/pylib/device/device_utils.py:1570: multiple_instances=False):
On 2015/07/24 14:03:10, perezju wrote:
> I don't think John will like adding an extra option (maybe he will prove me
> wrong?), and it leads to a somewhat messy implementation.
> 
> I propose having a GetProcessPidPairs (or something more artfully named), with
> essentially the same implementation as the current GetPids, but returning a
list
> of (binary_name, pid) pairs.
> 
> The implementation of GetPids would then just become:
> 
>   return dict(self.GetProcessPidPairs(...))
> 
> so we don't change the expectations of existing callers, and KillAll can use
the
> GetProcessPidPairs to really kill all matching processes.

Yeah I wasn't super happy about the flag either. Thanks for the great idea --
I'm stealing it :)

https://codereview.chromium.org/1254843002/diff/1/tools/telemetry/telemetry/i...
File tools/telemetry/telemetry/internal/platform/profiler/perf_profiler.py
(right):

https://codereview.chromium.org/1254843002/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/platform/profiler/perf_profiler.py:111:
device.KillAll(binary_name, signum=signal.SIGINT, blocking=True)
On 2015/07/24 14:03:10, perezju wrote:
> you should be able to pass quiet=True here

Yeah I thought about that but decided against it since we could silently fail in
cases where 'perf' fails to exit for some reason. WDYT?

[jbudorick, 2015-07-24 17:10:56]

+1, nice catch here.

https://codereview.chromium.org/1254843002/diff/20001/build/android/pylib/dev...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/1254843002/diff/20001/build/android/pylib/dev...
build/android/pylib/device/device_utils.py:1567: def _GetProcessPidPairs(self,
process_name):
I think I'd prefer making a breaking change to the GetPids interface by having
it return

{
  'process': [process_pid0, process_pid1, ...]
}

instead of adding a separate function. I imagine most clients of GetPids would
want to know all of the pids that a given process is using, not just an
arbitrary one.

[Sami, 2015-07-24 17:37:03]

On 2015/07/24 17:10:56, jbudorick wrote:
> +1, nice catch here.
> 
>
https://codereview.chromium.org/1254843002/diff/20001/build/android/pylib/dev...
> File build/android/pylib/device/device_utils.py (right):
> 
>
https://codereview.chromium.org/1254843002/diff/20001/build/android/pylib/dev...
> build/android/pylib/device/device_utils.py:1567: def _GetProcessPidPairs(self,
> process_name):
> I think I'd prefer making a breaking change to the GetPids interface by having
> it return
> 
> {
>   'process': [process_pid0, process_pid1, ...]
> }
> 
> instead of adding a separate function. I imagine most clients of GetPids would
> want to know all of the pids that a given process is using, not just an
> arbitrary one.

Good idea, done. Most of the clients seemed to care that there is at least one
PID and the rest were looking for a single one as far as I can tell. PTAL.

[perezju, 2015-07-27 09:56:32]

The new interface seems fine to me, in principle, but there are a few things
that worry me.

First, I think there is a small but non-zero chance that adding those asserts
will cause things to break. The checks should probably be there, it's bad that
we didn't have them before, but I think they should raise some kind of exception
with a clear error message ('At most one instance of process %s expected but
found pids: %s'), and we should keep an eye on bots after landing this.

The second bit that worries me, is that actually no client really wants what
GetPids is returning. We have effectively two kinds of clients:

1. I want to find all pids of processes that have "foo" somewhere in their name
(or just want check whether there are any of them running now).

2. I expect at most one instance of a process named "foobar" (with that exact
name) to be running, and I want to get its pid.

For clients expecting (1) they don't really care about grouping them by process
name (which has to be "undone" as in the latest KillAll implementation). So I
would still argue for just returning a list of (process_name, pid) pairs.

And there are enough clients wanting (2) that it's perhaps a good idea to
provide a "GetProcessPid" method to do exactly that.

What do you think?

https://codereview.chromium.org/1254843002/diff/40001/build/android/pylib/dev...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/1254843002/diff/40001/build/android/pylib/dev...
build/android/pylib/device/device_utils.py:741: for pid in pids]
maybe just me, but I think this would be somewhat more readable:

pids = itertools.chain.fromiterable(self.GetPids(process_name).itervalues())

https://codereview.chromium.org/1254843002/diff/40001/tools/telemetry/telemet...
File tools/telemetry/telemetry/internal/platform/profiler/tcpdump_profiler.py
(right):

https://codereview.chromium.org/1254843002/diff/40001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/profiler/tcpdump_profiler.py:46:
self._device.RunShellCommand('kill -term ' + tcpdump_pid)
str + int? I know it was like that before, but how does that even work?

[perezju, 2015-07-27 09:58:55]

btw, I think we also need to update the client here:
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...

[Sami, 2015-07-27 11:37:00]

https://codereview.chromium.org/1254843002/diff/40001/build/android/pylib/dev...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/1254843002/diff/40001/build/android/pylib/dev...
build/android/pylib/device/device_utils.py:741: for pid in pids]
On 2015/07/27 09:56:32, perezju wrote:
> maybe just me, but I think this would be somewhat more readable:
> 
> pids = itertools.chain.fromiterable(self.GetPids(process_name).itervalues())

Thanks, I went for a little less efficient but compact version.

https://codereview.chromium.org/1254843002/diff/40001/tools/telemetry/telemet...
File tools/telemetry/telemetry/internal/platform/profiler/tcpdump_profiler.py
(right):

https://codereview.chromium.org/1254843002/diff/40001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/profiler/tcpdump_profiler.py:46:
self._device.RunShellCommand('kill -term ' + tcpdump_pid)
On 2015/07/27 09:56:32, perezju wrote:
> str + int? I know it was like that before, but how does that even work?

GetPids returns the pids as strings. Not sure why but I don't particularly want
to change that with this patch.

[Sami, 2015-07-27 11:43:33]

On 2015/07/27 09:56:32, perezju wrote:
> The new interface seems fine to me, in principle, but there are a few things
> that worry me.
> 
> First, I think there is a small but non-zero chance that adding those asserts
> will cause things to break. The checks should probably be there, it's bad that
> we didn't have them before, but I think they should raise some kind of
exception
> with a clear error message ('At most one instance of process %s expected but
> found pids: %s'), and we should keep an eye on bots after landing this.

Agreed, done.

> The second bit that worries me, is that actually no client really wants what
> GetPids is returning. We have effectively two kinds of clients:
> 
> 1. I want to find all pids of processes that have "foo" somewhere in their
name
> (or just want check whether there are any of them running now).
> 
> 2. I expect at most one instance of a process named "foobar" (with that exact
> name) to be running, and I want to get its pid.
> 
> For clients expecting (1) they don't really care about grouping them by
process
> name (which has to be "undone" as in the latest KillAll implementation). So I
> would still argue for just returning a list of (process_name, pid) pairs.
> 
> And there are enough clients wanting (2) that it's perhaps a good idea to
> provide a "GetProcessPid" method to do exactly that.
> 
> What do you think?

I dunno, I think there's some value in forcing everyone to deal with the fact
that there may be multiple instances of the process they're looking for. We
don't really have enough information at the DeviceUtils level to decide what to
do in that case, and arbitrarily picking some strategy will just end up hiding
bugs.

> btw, I think we also need to update the client here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...

Yeah I saw that one but didn't think it needed changing -- do you think it does?

[perezju, 2015-07-27 12:51:37]

On 2015/07/27 11:43:33, Sami wrote:
> I dunno, I think there's some value in forcing everyone to deal with the fact
> that there may be multiple instances of the process they're looking for. We
> don't really have enough information at the DeviceUtils level to decide what
to
> do in that case, and arbitrarily picking some strategy will just end up hiding
> bugs.

Yep, we all agree we don't want silent bugs. What I was trying to go for is
not replicating the "if len(pids) > 1: raise Exception(...)" code everywhere,
and have that check done by a dedicated method. But if there are only a few
clients I guess we can live with that.

> > btw, I think we also need to update the client here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...
> 
> Yeah I saw that one but didn't think it needed changing -- do you think it
does?

Well the code will now end up comparing two lists of pids and, if there
happens to be more than one instance of process, the lists might or might not
be equal depending on the order in which we find their pids. Also the code as
written seems to expect only one chrome process to be running, but we don't make
any checks.

[Sami, 2015-07-27 14:17:25]

> Well the code will now end up comparing two lists of pids and, if there
> happens to be more than one instance of process, the lists might or might not
> be equal depending on the order in which we find their pids. Also the code as
> written seems to expect only one chrome process to be running, but we don't
make
> any checks.

Ah you're right, let me add a check for multiple processes which will also avoid
the ordering problem.

[jbudorick, 2015-07-27 15:25:32]

lgtm w/ nits

https://codereview.chromium.org/1254843002/diff/80001/build/android/pylib/mon...
File build/android/pylib/monkey/test_runner.py (right):

https://codereview.chromium.org/1254843002/diff/80001/build/android/pylib/mon...
build/android/pylib/monkey/test_runner.py:69: if len(before_pids[self._package])
> 1:
This should probably be before_pids.get(self._package, [])

https://codereview.chromium.org/1254843002/diff/80001/tools/telemetry/telemet...
File tools/telemetry/telemetry/internal/platform/profiler/perf_profiler.py
(right):

https://codereview.chromium.org/1254843002/diff/80001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/profiler/perf_profiler.py:110: if
device.GetPids(binary_name):
Why are you calling GetPids here? If you're trying to dodge the exception, you
could use KillAll's quiet flag...

[Sami, 2015-07-27 15:33:23]

https://codereview.chromium.org/1254843002/diff/80001/build/android/pylib/mon...
File build/android/pylib/monkey/test_runner.py (right):

https://codereview.chromium.org/1254843002/diff/80001/build/android/pylib/mon...
build/android/pylib/monkey/test_runner.py:69: if len(before_pids[self._package])
> 1:
On 2015/07/27 15:25:32, jbudorick wrote:
> This should probably be before_pids.get(self._package, [])

Done.

https://codereview.chromium.org/1254843002/diff/80001/tools/telemetry/telemet...
File tools/telemetry/telemetry/internal/platform/profiler/perf_profiler.py
(right):

https://codereview.chromium.org/1254843002/diff/80001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/profiler/perf_profiler.py:110: if
device.GetPids(binary_name):
On 2015/07/27 15:25:32, jbudorick wrote:
> Why are you calling GetPids here? If you're trying to dodge the exception, you
> could use KillAll's quiet flag...

Juan pointed this out too and looks like I misunderstood what |quiet| does. I
thought it wouldn't cause a failure if the process was found and subsequently
failed to exit. So both of you are right: quiet=True is what I want.

[Sami, 2015-07-27 16:12:34]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2015-07-27 16:12:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/1254843002/#ps100001
(title: "Review comments.")

[commit-bot: I haz the power, 2015-07-27 16:12:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1254843002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1254843002/100001

[commit-bot: I haz the power, 2015-07-27 17:36:47]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-07-27 17:37:43]

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