Simpler logic to setup/restore Chrome command line

Simpler logic to setup/restore Chrome command line

Simplified the logic to save, set, and restore the command line used to
launch a browser. For efficiency, functions are implemented as single
shell commands to run on the device.

This reduces about 20% of the time taken to start a new browser, and
overall 5% of the total running time of telemety unit tests.

BUG=379378
Committed: https://crrev.com/ed4ecc3210f219f40b77ce5efae5d668915b0ed1
Cr-Commit-Position: refs/heads/master@{#294401}

[perezju, 2014-09-05 12:51:16]

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

[perezju, 2014-09-05 12:51:17]

skyostil@chromium.org: Please review changes in 
tonyg@chromium.org: Please review changes in

[Sami, 2014-09-05 14:18:13]

Looks great. Couple of questions about the scripts.

https://codereview.chromium.org/547623002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py
(right):

https://codereview.chromium.org/547623002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:11:
import pipes
Imports in alphabetic order please.

https://codereview.chromium.org/547623002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:228: #
Values witout spaces, or that seem to be quoted are left untouched.
s/witout/without/

https://codereview.chromium.org/547623002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:253:
script = ('test -e {0}.telemetry.saved 2>/dev/null'
Too bad Android's mv doesn't support -n :P

Do we need the error redirections here though -- test should always be silent?

https://codereview.chromium.org/547623002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:270:
'mv {0}.telemetry.saved {0} 2>/dev/null').format(cmdline_file)
Should we do the test -e trick here too so we can avoid the error redirect?

[tonyg, 2014-09-05 16:16:08]

I'm super-excited to see these cycle-time improvements going by. However, I do
worry about the code maintainability if we keep moving this direction. We're
moving from easy to test/debug/read python to harder-to-read shell scripts. John
pushed back on several similar changes I tried to make -- and I'm starting to
see why. No single change is that problematic, but I see 3 of them pending now
and in the limit we'd just end up with a ball of shell script.

The root of the problem you're papering over here is the overhead of starting an
adb shell. If we could fix that, then we no longer have to worry as much about
how many commands we run. Marcus had a nice proof-of-concept here:
https://code.google.com/p/chromium/issues/detail?id=379378#c8

What do you think about trying to make something like that landable and then
re-investigating whether these sorts of hacks are still worthwhile? We could
also investigate the performance of
https://github.com/google/python-adb/tree/master . I suspect it'd have similar
wins to Marcus' approach despite the comment about slower perf due to python.

If, ultimately, we still want shell scripts to perform certain hot operations
more efficiently, I recommend we encapsulate them in pylib so that we can get
rock solid implementations that all consumers can benefit from without exposing
each consumer to rolling their own sh.

Thoughts?

[jbudorick, 2014-09-05 16:31:32]

On 2014/09/05 16:16:08, tonyg wrote:
> I'm super-excited to see these cycle-time improvements going by. However, I do
> worry about the code maintainability if we keep moving this direction. We're
> moving from easy to test/debug/read python to harder-to-read shell scripts.
John
> pushed back on several similar changes I tried to make -- and I'm starting to
> see why. No single change is that problematic, but I see 3 of them pending now
> and in the limit we'd just end up with a ball of shell script.
> 

Yeah, this is exactly the issue. Things can get out of hand _quickly_.

> The root of the problem you're papering over here is the overhead of starting
an
> adb shell. If we could fix that, then we no longer have to worry as much about
> how many commands we run. Marcus had a nice proof-of-concept here:
> https://code.google.com/p/chromium/issues/detail?id=379378#c8
> 
> What do you think about trying to make something like that landable and then
> re-investigating whether these sorts of hacks are still worthwhile? We could
> also investigate the performance of
> https://github.com/google/python-adb/tree/master . I suspect it'd have similar
> wins to Marcus' approach despite the comment about slower perf due to python.
> 

I reached out to the current owner of python-adb earlier this week. I asked him
specifically about the performance penalty and he said that it isn't actually
much of a problem. He noted two big advantages to the library over calling adb
via shell:
 - Much better reliability in setups with multiple devices attached to the same
host
 - Much better error reporting, since they can get exceptions rather than having
to parse stdout & return codes

There are some other things in the works here w.r.t. modifying adb in the AOSP
mainline (...), but I think that python-adb could be a viable way forward.

> If, ultimately, we still want shell scripts to perform certain hot operations
> more efficiently, I recommend we encapsulate them in pylib so that we can get
> rock solid implementations that all consumers can benefit from without
exposing
> each consumer to rolling their own sh.
> 

+1. I'd be fine with handrolling shell scripts if we keep them contained, but I
don't want everyone abusing RunShellCommand all over the place.

> Thoughts?

[slamm, 2014-09-05 17:14:01]

slamm@chromium.org changed reviewers:
+ slamm@chromium.org

[slamm, 2014-09-05 17:14:02]

https://codereview.chromium.org/547623002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py
(right):

https://codereview.chromium.org/547623002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:239:
return '%s=%s' % (key, pipes.quote(values))
FYI, RunShellCommand doc string says, the command "must not include the single
quotes as we use them to escape the whole command." However, it looks like the
single quotes do get properly quoted.

https://codereview.chromium.org/547623002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:242:
content = ' '.join([QuoteIfNeeded(arg) for arg in args])
Minor suggestion...

Use a generator expression instead of a list comprehension.

content = ' '.join(QuoteIfNeeded(arg) for arg in args)

https://codereview.chromium.org/547623002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:247: #
prevent Telemetry created command lines to end up mistankely saved and
mistakenly

https://codereview.chromium.org/547623002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:265: #
restore it.
How about making this comment the function doc string?

[perezju, 2014-09-08 09:03:42]

Thanks all for the helpful comments. I actually agree with much of what has been
said, in particular about not peppering the code with shell scripts all over the
place.

Regarding improvements at the level of adb SendCommand, before any of this I did
ran some experiments using Marcus proof of concept, which indeed nearly halves
the time spent on SendCommand but, overall, only reduces the total running time
of Telemetry unittests in some 5-7%. Which is something I think we should
definitely do anyway, as the benefits propagate more widely, but will also
require a much larger amount of effort.

What I've been trying to do here is looking at some other orthogonal time
savings that we could get. By clearing up a bit the logic on some of the
currently most time consuming operations, and avoid sending commands which are
actually not needed.

What I plan to do about the current issue is try again and still propose a
better implementation of the logic to set/restore the contents of a file, but
using the high-level adb interface rather than low-level shell commands (this
actually was my first approach, but did not save as much time).

On a side note, what would people think about a couple additions to the
adb.device() interface:
- A `WriteShortFile`, to be used when writing files which only contain a short
string, internally implemented using just `echo` rather than having to push a
file to the device.
- A `ReplaceFile`, which writes new contents to a file, but also reads and
returns the previous contents of the file being replaced.
These would be particularly useful to provide a quicker implementation for this
issue, but I also see them being applicable more widely.

[perezju, 2014-09-08 13:43:53]

I've added a new patch set with an example of a `WriteText` function to write
short text strings into files (still thinking about the most appropriate name).
This is faster than pushing files to the device (specially when writing
`as_root`, since this currently involves pushing two files for every file we
want to write, c.f. SetProtectedFileContents), and automatically selects root if
available. Thus the command is successful both if: (a) we have root access to
the device, or (b) we don't have root access but the file is not protected.

Ideally, if this seems like a good idea, I would like to move this function
definition out of android_browser_backend.py and into
build/android/pylib/device/device_utils.py.

[perezju, 2014-09-08 13:45:03]

https://codereview.chromium.org/547623002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py
(right):

https://codereview.chromium.org/547623002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:239:
return '%s=%s' % (key, pipes.quote(values))
On 2014/09/05 17:14:01, slamm wrote:
> FYI, RunShellCommand doc string says, the command "must not include the single
> quotes as we use them to escape the whole command." However, it looks like the
> single quotes do get properly quoted.

Acknowledged. Perhaps the comment in RunShellCommand should be edited?

[perezju, 2014-09-11 12:08:17]

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

[perezju, 2014-09-11 12:08:18]

jbudorick@chromium.org: Please review changes in DeviceUtils

I've added a WriteTextFile in device utils for writing short text files to the
device, it's implemented using echo rather than push, so significantly faster
than WriteFile.

This currently yields about 3% speedup running all telemetry tests.

[jbudorick, 2014-09-11 13:57:07]

b/a/pylib/device lgtm w/ nit

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

https://codereview.chromium.org/547623002/diff/40001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:640: retries=None):
nit: line this up with the params above

[tonyg, 2014-09-11 15:25:34]

lgtm

[perezju, 2014-09-11 15:28:58]

The CQ bit was checked by perezju@chromium.org

[commit-bot: I haz the power, 2014-09-11 15:30:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/547623002/60001

[commit-bot: I haz the power, 2014-09-11 16:40:04]

Committed patchset #4 (id:60001) as 30173ba3d066860e0dfcf8262d33b127d592da46

[commit-bot: I haz the power, 2014-09-11 16:45:24]

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