Android: start upstreaming some of our perf tests.

Android: start upstreaming some of our perf tests.

- fix some "forwarder" issues on chrome_remote_control.
- adds a "platform" interface to crc so that tests can grab stats from
the platform, where possible.
- integrates "surface stats collector" with scrolling tests.

BUG=155678, 11187036, 138479
TEST=./tools/perf/run_tests --browser=android-chrome testScrolling

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=163361

[bulach, 2012-10-17 19:30:58]

this is a work-in-progress-patch :)

a few open questions for nat / tonyg:
- how does chrome_remote_control integrate with buildbot to print / parse /
graph perf results? the "surface_stats_collector.py" itself collects and prints
in a "buildbot-friendly" way, but it'd be nice follow whatever CRC does..

- I added this "platform" concept to CRC, not sure if that's a good name / fit
as part of the "browser" object.

sami / wangxianzhu: the surface_stats_collector.py is a verbatim copy of
downstream at this point, but adding you both since it seems you're the owners
for it..

thanks!

[tonyg, 2012-10-18 00:11:05]

On 2012/10/17 19:30:58, bulach wrote:
> this is a work-in-progress-patch :)

Wow! That was fast :)

I went ahead and pulled out the forwarder fix to another patch to get that going
again.

> 
> a few open questions for nat / tonyg:
> - how does chrome_remote_control integrate with buildbot to print / parse /
> graph perf results? the "surface_stats_collector.py" itself collects and
prints
> in a "buildbot-friendly" way, but it'd be nice follow whatever CRC does..

That's all that is needed. It keeps the same stdio buildbot API for now. So this
is perfect.

> 
> - I added this "platform" concept to CRC, not sure if that's a good name / fit
> as part of the "browser" object.
> 

I like the platform abstraction. Hopefully there's a way to implement this on
other platforms as well.

> sami / wangxianzhu: the surface_stats_collector.py is a verbatim copy of
> downstream at this point, but adding you both since it seems you're the owners
> for it..
> 
> thanks!

[bulach, 2012-10-18 11:11:45]

thanks tony!
duh, I was running the tests for these, not the real perf suite.. :)

for my own information, this:
./tools/perf/run_multipage_benchmarks --browser=android-chrome --show-stdout
scrolling_benchmark tools/perf/page_sets/animations/animations.json

prints all results we need, now extended with the surface stats, yay!

I've rebased against Tony's patch, and I guess this is ready to be looked into.

[Sami, 2012-10-18 11:35:57]

http://codereview.chromium.org/11187036/diff/1/build/android/pylib/surface_st...
File build/android/pylib/surface_stats_collector.py (right):

http://codereview.chromium.org/11187036/diff/1/build/android/pylib/surface_st...
build/android/pylib/surface_stats_collector.py:72: renderer_timestamps =
self._CollectSurfaceTextureTimestamps()
This bit depends on the browser sending us timestamps from the SurfaceTexture.
Since it seems like we're moving away from the SurfaceTexture architecture, I'd
drop this part.

That's not to say the data it collects isn't useful. I think we should have a
way to measure various internal compositor latencies once we dust settles on the
architecture front.

[bulach, 2012-10-18 12:33:56]

http://codereview.chromium.org/11187036/diff/1/build/android/pylib/surface_st...
File build/android/pylib/surface_stats_collector.py (right):

http://codereview.chromium.org/11187036/diff/1/build/android/pylib/surface_st...
build/android/pylib/surface_stats_collector.py:72: renderer_timestamps =
self._CollectSurfaceTextureTimestamps()
On 2012/10/18 11:35:57, Sami wrote:
> This bit depends on the browser sending us timestamps from the SurfaceTexture.
> Since it seems like we're moving away from the SurfaceTexture architecture,
I'd
> drop this part.
> 
> That's not to say the data it collects isn't useful. I think we should have a
> way to measure various internal compositor latencies once we dust settles on
the
> architecture front.

thanks for the clarification sami! it simplifies this class a lot!

[tonyg, 2012-10-19 00:47:19]

Everything else looks good to me.

http://codereview.chromium.org/11187036/diff/1011/build/android/pylib/surface...
File build/android/pylib/surface_stats_collector.py (right):

http://codereview.chromium.org/11187036/diff/1011/build/android/pylib/surface...
build/android/pylib/surface_stats_collector.py:38: def Start(self):
Just a suggestion, feel free to ignore, but what do you think about going with
__enter__ and __exit__ instead of Start and Stop. Then we can change all the
users to use it via a with-block.

http://codereview.chromium.org/11187036/diff/1011/tools/chrome_remote_control...
File tools/chrome_remote_control/chrome_remote_control/adb_commands.py (right):

http://codereview.chromium.org/11187036/diff/1011/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/adb_commands.py:139:
port_pairs = [(0, host_port), ]
Why the 0? I didn't need this in my patch.

http://codereview.chromium.org/11187036/diff/1011/tools/perf/perf_tools/scrol...
File tools/perf/perf_tools/scrolling_benchmark.py (right):

http://codereview.chromium.org/11187036/diff/1011/tools/perf/perf_tools/scrol...
tools/perf/perf_tools/scrolling_benchmark.py:44:
tab.browser.platform.StartSurfaceCollector()
Move this below Execute(scroll_js) while keeping it above the
Execute(start_scroll_js)'s.

[bulach, 2012-10-22 15:57:14]

thanks tony! all comments addressed, another look please?

http://codereview.chromium.org/11187036/diff/1011/build/android/pylib/surface...
File build/android/pylib/surface_stats_collector.py (right):

http://codereview.chromium.org/11187036/diff/1011/build/android/pylib/surface...
build/android/pylib/surface_stats_collector.py:38: def Start(self):
On 2012/10/19 00:47:19, tonyg wrote:
> Just a suggestion, feel free to ignore, but what do you think about going with
> __enter__ and __exit__ instead of Start and Stop. Then we can change all the
> users to use it via a with-block.

that's a pretty cool trick!! I wasn't aware of this RAII pattern in python,
thanks!
done.

http://codereview.chromium.org/11187036/diff/1011/tools/chrome_remote_control...
File tools/chrome_remote_control/chrome_remote_control/adb_commands.py (right):

http://codereview.chromium.org/11187036/diff/1011/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/adb_commands.py:139:
port_pairs = [(0, host_port), ]
On 2012/10/19 00:47:19, tonyg wrote:
> Why the 0? I didn't need this in my patch.

0 means dynamically allocating the device port. since we're getting it on 149
anyways, seems better..

http://codereview.chromium.org/11187036/diff/1011/tools/perf/perf_tools/scrol...
File tools/perf/perf_tools/scrolling_benchmark.py (right):

http://codereview.chromium.org/11187036/diff/1011/tools/perf/perf_tools/scrol...
tools/perf/perf_tools/scrolling_benchmark.py:44:
tab.browser.platform.StartSurfaceCollector()
On 2012/10/19 00:47:19, tonyg wrote:
> Move this below Execute(scroll_js) while keeping it above the
> Execute(start_scroll_js)'s.

Done.

[tonyg, 2012-10-22 17:21:27]

Just one question. Everything else looks good.

http://codereview.chromium.org/11187036/diff/11001/tools/chrome_remote_contro...
File tools/chrome_remote_control/chrome_remote_control/adb_commands.py (right):

http://codereview.chromium.org/11187036/diff/11001/tools/chrome_remote_contro...
tools/chrome_remote_control/chrome_remote_control/adb_commands.py:135:
port_pairs = [(0, host_port), ]
Are you sure this works? I wouldn't expect it to.

When web page replay runs (only if you have src-internal checked out),
--testing-fixed-http-port causes all http requests to go to a fixed port. If the
port on the device doesn't match that, I don't see how replaying would work.

[bulach, 2012-10-22 17:29:02]

thanks tony! inline:

http://codereview.chromium.org/11187036/diff/11001/tools/chrome_remote_contro...
File tools/chrome_remote_control/chrome_remote_control/adb_commands.py (right):

http://codereview.chromium.org/11187036/diff/11001/tools/chrome_remote_contro...
tools/chrome_remote_control/chrome_remote_control/adb_commands.py:135:
port_pairs = [(0, host_port), ]
On 2012/10/22 17:21:28, tonyg wrote:
> Are you sure this works? I wouldn't expect it to.
> 
> When web page replay runs (only if you have src-internal checked out),
> --testing-fixed-http-port causes all http requests to go to a fixed port. If
the
> port on the device doesn't match that, I don't see how replaying would work.

that's a good point. it worked for me, but you convinced me it'd break other
cases.. I just reverted this file then.

[tonyg, 2012-10-22 17:45:41]

lgtm

[bulach, 2012-10-22 18:03:05]

thanks tony! are there any specific try bots I should trigger or is the regular
CQ enough?

[tonyg, 2012-10-22 18:13:07]

Nope, just the CQ. The unittests aren't being run on any bots yet, but
we'd like to change that.

On Mon, Oct 22, 2012 at 11:03 AM,  <bulach@chromium.org> wrote:
> thanks tony! are there any specific try bots I should trigger or is the
> regular
> CQ enough?
>
> http://codereview.chromium.org/11187036/

[commit-bot: I haz the power, 2012-10-22 18:15:21]

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

[commit-bot: I haz the power, 2012-10-22 20:21:55]

Change committed as 163361

[nduca, 2012-12-04 22:39:31]

i'm really confused about this vs the current platform vs platform_harness
(https://codereview.chromium.org/11428107/). This smells. We need to do cleaner.