Relands Android perf tests: kill adbd on the device after running a test.

Relands Android perf tests: kill adbd on the device after running a test.

Third retry on crrev.com/226762.
Adds "wait-for-device".
There's some data indicating that adbd on the device sometimes
spontaneously restores its connection.
After running a test, let's kill adbd and wait for bit.

This is a speculative change to try to make the bots healthier.

BUG=268450
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229931

[bulach, 2013-10-21 10:26:29]

ptal, perhaps third time lucky :)
major differences:
- use wait-for-device
- kill adbd before running the test, not afterwards, so all tests are guaranteed
to be started from "clean"-ish..

[rmcilroy, 2013-10-21 11:33:13]

https://codereview.chromium.org/32163002/diff/40001/build/android/pylib/andro...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/32163002/diff/40001/build/android/pylib/andro...
build/android/pylib/android_commands.py:461: def KillAdbdDevice(self):
Nit - how about RestartAdbdOnDevice()?

https://codereview.chromium.org/32163002/diff/40001/build/android/pylib/andro...
build/android/pylib/android_commands.py:466: logging.info('Waiting for device to
settle...')
Nit - "Waiting for adbd to restart on device..." ?  Also, is there any way to
get the serial number here (and above in "killing...")?

https://codereview.chromium.org/32163002/diff/40001/build/android/pylib/perf/...
File build/android/pylib/perf/test_runner.py (right):

https://codereview.chromium.org/32163002/diff/40001/build/android/pylib/perf/...
build/android/pylib/perf/test_runner.py:130: time.sleep(5)
Is this necesary now we have --wait-for-device? I'd prefer we didn't have to add
another 5 second per test delay to the bots if possible.

[bulach, 2013-10-21 15:30:13]

+pliard, I found the issue :)

what happens:
- kill adb on the device
- it restarts, and connects to a different port
- however, forwarder2 on the host has a map from device => port, which it keeps
regardless of the above.

With this change, after the last port mapping is removed, it'll also remove the
"adb forward" port and the internal forwarder2 mapping... ptal!

https://codereview.chromium.org/32163002/diff/40001/build/android/pylib/andro...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/32163002/diff/40001/build/android/pylib/andro...
build/android/pylib/android_commands.py:461: def KillAdbdDevice(self):
On 2013/10/21 11:33:13, rmcilroy wrote:
> Nit - how about RestartAdbdOnDevice()?

Done.

https://codereview.chromium.org/32163002/diff/40001/build/android/pylib/andro...
build/android/pylib/android_commands.py:466: logging.info('Waiting for device to
settle...')
On 2013/10/21 11:33:13, rmcilroy wrote:
> Nit - "Waiting for adbd to restart on device..." ?  Also, is there any way to
> get the serial number here (and above in "killing...")?

yes! :) thankfully, it's already done at a lower level:
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...

https://codereview.chromium.org/32163002/diff/40001/build/android/pylib/perf/...
File build/android/pylib/perf/test_runner.py (right):

https://codereview.chromium.org/32163002/diff/40001/build/android/pylib/perf/...
build/android/pylib/perf/test_runner.py:130: time.sleep(5)
On 2013/10/21 11:33:13, rmcilroy wrote:
> Is this necesary now we have --wait-for-device? I'd prefer we didn't have to
add
> another 5 second per test delay to the bots if possible.

Done.

[Philippe, 2013-10-21 15:44:58]

Nice Marcus, thanks! LGTM modulo the question I have on line 232.

https://codereview.chromium.org/32163002/diff/100001/tools/android/forwarder2...
File tools/android/forwarder2/host_forwarder_main.cc (right):

https://codereview.chromium.org/32163002/diff/100001/tools/android/forwarder2...
tools/android/forwarder2/host_forwarder_main.cc:232: if
(others->first.find_first_of(prefix) == 0U)
Not sure I read the find_first_of() documentation properly or I understand what
you are doing here correctly but did you mean to use instead the 3rd version of
string::find()?: http://www.cplusplus.com/reference/string/string/find/

My understanding is that the find_first_of() would return 0 if any of the
characters in |prefix| is in |others->first|. Is that what you meant?

https://codereview.chromium.org/32163002/diff/100001/tools/android/forwarder2...
tools/android/forwarder2/host_forwarder_main.cc:243: device_serial.empty() ? ""
: serial_part.c_str(),
Nit: I guess that you can apply the fix here that you did on line 262 :)

[Philippe, 2013-10-21 15:53:20]

https://codereview.chromium.org/32163002/diff/100001/tools/android/forwarder2...
File tools/android/forwarder2/host_forwarder_main.cc (right):

https://codereview.chromium.org/32163002/diff/100001/tools/android/forwarder2...
tools/android/forwarder2/host_forwarder_main.cc:232: if
(others->first.find_first_of(prefix) == 0U)
On 2013/10/21 15:44:58, Philippe wrote:
> Not sure I read the find_first_of() documentation properly or I understand
what
> you are doing here correctly but did you mean to use instead the 3rd version
of
> string::find()?: http://www.cplusplus.com/reference/string/string/find/
> 
> My understanding is that the find_first_of() would return 0 if any of the
> characters in |prefix| is in |others->first|. Is that what you meant? 

Small correction in my last paragraph: s/others->first/others->first[0].

[bulach, 2013-10-21 17:19:59]

thanks pliard! another look please?

https://codereview.chromium.org/32163002/diff/100001/tools/android/forwarder2...
File tools/android/forwarder2/host_forwarder_main.cc (right):

https://codereview.chromium.org/32163002/diff/100001/tools/android/forwarder2...
tools/android/forwarder2/host_forwarder_main.cc:232: if
(others->first.find_first_of(prefix) == 0U)
On 2013/10/21 15:53:21, Philippe wrote:
> On 2013/10/21 15:44:58, Philippe wrote:
> > Not sure I read the find_first_of() documentation properly or I understand
> what
> > you are doing here correctly but did you mean to use instead the 3rd version
> of
> > string::find()?: http://www.cplusplus.com/reference/string/string/find/
> > 
> > My understanding is that the find_first_of() would return 0 if any of the
> > characters in |prefix| is in |others->first|. Is that what you meant? 
> 
> Small correction in my last paragraph: s/others->first/others->first[0].

yeah, that's exactly what I wanted, sorry for the bad mistake! thanks!! :)

https://codereview.chromium.org/32163002/diff/100001/tools/android/forwarder2...
tools/android/forwarder2/host_forwarder_main.cc:243: device_serial.empty() ? ""
: serial_part.c_str(),
On 2013/10/21 15:44:58, Philippe wrote:
> Nit: I guess that you can apply the fix here that you did on line 262 :)

Done.

[Philippe, 2013-10-21 17:25:23]

Thanks Marcus! Still LGTM!

https://codereview.chromium.org/32163002/diff/100001/tools/android/forwarder2...
File tools/android/forwarder2/host_forwarder_main.cc (right):

https://codereview.chromium.org/32163002/diff/100001/tools/android/forwarder2...
tools/android/forwarder2/host_forwarder_main.cc:232: if
(others->first.find_first_of(prefix) == 0U)
On 2013/10/21 17:19:59, bulach wrote:
> On 2013/10/21 15:53:21, Philippe wrote:
> > On 2013/10/21 15:44:58, Philippe wrote:
> > > Not sure I read the find_first_of() documentation properly or I understand
> > what
> > > you are doing here correctly but did you mean to use instead the 3rd
version
> > of
> > > string::find()?: http://www.cplusplus.com/reference/string/string/find/
> > > 
> > > My understanding is that the find_first_of() would return 0 if any of the
> > > characters in |prefix| is in |others->first|. Is that what you meant? 
> > 
> > Small correction in my last paragraph: s/others->first/others->first[0].
> 
> yeah, that's exactly what I wanted, sorry for the bad mistake! thanks!! :)

No worries, seems that I wasn't right either with the 3rd version of find() :)

[rmcilroy, 2013-10-21 17:34:43]

Nice, lgtm with one nit.

https://codereview.chromium.org/32163002/diff/210001/build/android/pylib/perf...
File build/android/pylib/perf/test_runner.py (right):

https://codereview.chromium.org/32163002/diff/210001/build/android/pylib/perf...
build/android/pylib/perf/test_runner.py:49: import time
I think you can get rid of this "time" import now.

[tonyg, 2013-10-21 17:48:38]

lgtm

https://codereview.chromium.org/32163002/diff/210001/build/android/pylib/andr...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/32163002/diff/210001/build/android/pylib/andr...
build/android/pylib/android_commands.py:464: if adb_pids:
Should we assert adb_pids? There is no way we'd be able to run this command and
get a result if adbd is not running.

https://codereview.chromium.org/32163002/diff/210001/build/android/pylib/andr...
build/android/pylib/android_commands.py:465: self.RunShellCommandWithSU('kill
%s' % ' '.join(adb_pids))
We already have a KillAll() method in this class. Can we make it smart enough to
know when it needs to use su and then rely on it instead of duplicating?

[bulach, 2013-10-21 18:49:39]

thanks everyone! all comments addressed, I'll put this through the CQ so
hopefully by tomorrow (my) morning I'll get some results...

https://codereview.chromium.org/32163002/diff/210001/build/android/pylib/andr...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/32163002/diff/210001/build/android/pylib/andr...
build/android/pylib/android_commands.py:464: if adb_pids:
On 2013/10/21 17:48:39, tonyg wrote:
> Should we assert adb_pids? There is no way we'd be able to run this command
and
> get a result if adbd is not running.

Done.

https://codereview.chromium.org/32163002/diff/210001/build/android/pylib/andr...
build/android/pylib/android_commands.py:465: self.RunShellCommandWithSU('kill
%s' % ' '.join(adb_pids))
On 2013/10/21 17:48:39, tonyg wrote:
> We already have a KillAll() method in this class. Can we make it smart enough
to
> know when it needs to use su and then rely on it instead of duplicating?

Done.

https://codereview.chromium.org/32163002/diff/210001/build/android/pylib/perf...
File build/android/pylib/perf/test_runner.py (right):

https://codereview.chromium.org/32163002/diff/210001/build/android/pylib/perf...
build/android/pylib/perf/test_runner.py:49: import time
On 2013/10/21 17:34:43, rmcilroy wrote:
> I think you can get rid of this "time" import now.

Done (and android_commands too..)

[commit-bot: I haz the power, 2013-10-21 18:50:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/32163002/310001

[commit-bot: I haz the power, 2013-10-21 21:56:00]

Change committed as 229931