Input latency performance test that uses tracing.

Input latency performance test that uses tracing.

This CL adds a new gyp target called performance_browser_tests to run the new input latency test as a browser test.

See the comments in latency_tests.cc for an overview of how latency is measured.

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

[jbates, 2011-12-08 02:11:18]

Work in progress... This is producing some really interesting numbers locally. I
still need to figure out how to send this data to the perf bots. Also there are
some more combinations (software, etc) that need to be measured.

[jbates, 2011-12-20 22:40:07]

PTAL

nduca: test implementation
cmp: performance_browser_tests and browser_perf_test.h/cc
phajdan: chrome/test OWNERS
apatrick: gpu OWNERS
jam: content OWNERS

[jam, 2011-12-20 22:44:41]

content lgtm

[apatrick_chromium, 2011-12-20 23:01:42]

gpu lgtm

[nduca, 2011-12-20 23:37:42]

will do detailed review later but wrt this new latency directory and the
existing frame_rate directory, maybe it would  make sense to build a general
directory called 'gpu' or 'rendering' and put everything in there, and setup gyp
accordingly? Or is there something special about latency/ that I'm not getting?

[cmp, 2011-12-20 23:44:51]

lgtm with nits

http://codereview.chromium.org/8883005/diff/18001/chrome/test/perf/browser_pe...
File chrome/test/perf/browser_perf_test.cc (right):

http://codereview.chromium.org/8883005/diff/18001/chrome/test/perf/browser_pe...
chrome/test/perf/browser_perf_test.cc:11: //#include
"chrome/test/perf/perf_test.h"
please uncomment

http://codereview.chromium.org/8883005/diff/18001/chrome/test/perf/browser_pe...
chrome/test/perf/browser_perf_test.cc:27:
//perf_test::PrintIOPerfInfo(test_name, chrome_processes, browser_pid);
this should be uncommented

http://codereview.chromium.org/8883005/diff/18001/chrome/test/perf/browser_pe...
chrome/test/perf/browser_perf_test.cc:33:
//perf_test::PrintMemoryUsageInfo(test_name, chrome_processes, browser_pid);
this should be uncommented

http://codereview.chromium.org/8883005/diff/18001/chrome/test/perf/latency/la...
File chrome/test/perf/latency/latency_tests.cc (right):

http://codereview.chromium.org/8883005/diff/18001/chrome/test/perf/latency/la...
chrome/test/perf/latency/latency_tests.cc:430: 
remove empty line

http://codereview.chromium.org/8883005/diff/18001/chrome/test/perf/latency/la...
chrome/test/perf/latency/latency_tests.cc:455: 
remove empty line

http://codereview.chromium.org/8883005/diff/18001/chrome/test/perf/latency/la...
chrome/test/perf/latency/latency_tests.cc:457: 
remove empty line

http://codereview.chromium.org/8883005/diff/18001/chrome/test/perf/latency/la...
chrome/test/perf/latency/latency_tests.cc:543: 
remove empty line

[nduca, 2011-12-21 00:10:15]

Based on an offline discussion, lets try a chome/test/perf/rendering for your
latency stuff. Get the old frame_rate owners into the new directory, too, and we
can do followon CLs to move existing code into this new happy place.

[jbates, 2011-12-21 00:39:59]

As discussed, moved latency/ to chrome/test/perf/rendering/ where we will
continue our rendering perf test suite. A followup CL will move frame_rate/
contents to rendering/.

http://codereview.chromium.org/8883005/diff/18001/chrome/test/perf/browser_pe...
File chrome/test/perf/browser_perf_test.cc (right):

http://codereview.chromium.org/8883005/diff/18001/chrome/test/perf/browser_pe...
chrome/test/perf/browser_perf_test.cc:11: //#include
"chrome/test/perf/perf_test.h"
On 2011/12/20 23:44:51, cmp wrote:
> please uncomment

Done.

http://codereview.chromium.org/8883005/diff/18001/chrome/test/perf/browser_pe...
chrome/test/perf/browser_perf_test.cc:27:
//perf_test::PrintIOPerfInfo(test_name, chrome_processes, browser_pid);
On 2011/12/20 23:44:51, cmp wrote:
> this should be uncommented

Done.

http://codereview.chromium.org/8883005/diff/18001/chrome/test/perf/browser_pe...
chrome/test/perf/browser_perf_test.cc:33:
//perf_test::PrintMemoryUsageInfo(test_name, chrome_processes, browser_pid);
On 2011/12/20 23:44:51, cmp wrote:
> this should be uncommented

Done.

http://codereview.chromium.org/8883005/diff/18001/chrome/test/perf/latency/la...
File chrome/test/perf/latency/latency_tests.cc (right):

http://codereview.chromium.org/8883005/diff/18001/chrome/test/perf/latency/la...
chrome/test/perf/latency/latency_tests.cc:430: 
On 2011/12/20 23:44:51, cmp wrote:
> remove empty line

Done.

http://codereview.chromium.org/8883005/diff/18001/chrome/test/perf/latency/la...
chrome/test/perf/latency/latency_tests.cc:455: 
On 2011/12/20 23:44:51, cmp wrote:
> remove empty line

Done.

http://codereview.chromium.org/8883005/diff/18001/chrome/test/perf/latency/la...
chrome/test/perf/latency/latency_tests.cc:457: 
On 2011/12/20 23:44:51, cmp wrote:
> remove empty line

Done.

http://codereview.chromium.org/8883005/diff/18001/chrome/test/perf/latency/la...
chrome/test/perf/latency/latency_tests.cc:543: 
On 2011/12/20 23:44:51, cmp wrote:
> remove empty line

Done.

[cmp, 2011-12-21 07:41:40]

lgtm

[Paweł Hajdan Jr., 2011-12-21 09:50:28]

I didn't really review in the detail, what I'm mostly worried about is the
Sleep.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
File chrome/test/perf/rendering/latency_tests.cc (right):

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:97: virtual void
SetUpCommandLine(CommandLine* command_line) {
nit: OVERRIDE

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:101: #ifdef
TEST_THREADED_COMPOSITING_LATENCY
Why is this pre-processor controlled? I don't see
TEST_THREADED_COMPOSITING_LATENCY in any other place. Why not just pass the
command-line flag?

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:123: private:
nit: Indent +1.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:180: EXPECT_LT(num_frames_ -
num_frames_/4, static_cast<int>(events.size()));
nit: Spaces around "/".

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:210: else if (events[i]->name ==
"DoClear") {
nit: move this to the previous line, same below, i.e.:

if (...) {
} else if (...) {
} else {
}

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:231: FilePath test_path_;
nit: Please comment member variables.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:285:
PathService::Get(chrome::DIR_TEST_DATA, &test_path_);
Check the return value.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:327:
base::PlatformThread::Sleep(200);
Shouldn't you rather wait for an event or something? Is the pipeline
_guaranteed_ to be flushed in 2 s? Why not 1s then? Or on a slow machine?

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:406: else if (mode_ == "software") {
nit: Move to the previous line.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:409: CHECK(0) << "invalid mode";
Oh, a CHECK? We usually do a NOTREACHED() in those cases.

[jbates, 2011-12-21 19:34:18]

Thanks Pawel, got it cleaned up.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
File chrome/test/perf/rendering/latency_tests.cc (right):

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:97: virtual void
SetUpCommandLine(CommandLine* command_line) {
On 2011/12/21 09:50:29, Paweł Hajdan Jr. wrote:
> nit: OVERRIDE

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:101: #ifdef
TEST_THREADED_COMPOSITING_LATENCY
On 2011/12/21 09:50:29, Paweł Hajdan Jr. wrote:
> Why is this pre-processor controlled? I don't see
> TEST_THREADED_COMPOSITING_LATENCY in any other place. Why not just pass the
> command-line flag?

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:123: private:
On 2011/12/21 09:50:29, Paweł Hajdan Jr. wrote:
> nit: Indent +1.

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:180: EXPECT_LT(num_frames_ -
num_frames_/4, static_cast<int>(events.size()));
On 2011/12/21 09:50:29, Paweł Hajdan Jr. wrote:
> nit: Spaces around "/".

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:210: else if (events[i]->name ==
"DoClear") {
On 2011/12/21 09:50:29, Paweł Hajdan Jr. wrote:
> nit: move this to the previous line, same below, i.e.:
> 
> if (...) {
> } else if (...) {
> } else {
> }

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:231: FilePath test_path_;
On 2011/12/21 09:50:29, Paweł Hajdan Jr. wrote:
> nit: Please comment member variables.

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:285:
PathService::Get(chrome::DIR_TEST_DATA, &test_path_);
On 2011/12/21 09:50:29, Paweł Hajdan Jr. wrote:
> Check the return value.

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:327:
base::PlatformThread::Sleep(200);
On 2011/12/21 09:50:29, Paweł Hajdan Jr. wrote:
> Shouldn't you rather wait for an event or something? Is the pipeline
> _guaranteed_ to be flushed in 2 s? Why not 1s then? Or on a slow machine?

You're right, this was left-over from local testing. No sleep is needed here,
because the analysis already skips the first and last few frames. I also updated
the html to call gl.finish() before ending the test which will further reduce
the chance of data from a previous test showing up in the next trace.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:406: else if (mode_ == "software") {
On 2011/12/21 09:50:29, Paweł Hajdan Jr. wrote:
> nit: Move to the previous line.

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:409: CHECK(0) << "invalid mode";
On 2011/12/21 09:50:29, Paweł Hajdan Jr. wrote:
> Oh, a CHECK? We usually do a NOTREACHED() in those cases.

Done.

[nduca, 2011-12-21 20:24:23]

Two big questions:
1. How are we going to extend this test to test latency of scrolling on the
compositor thread? Frames will be generated that will never even hit the main
thread.

2. Is tracing really the right way to measure this? I know we embarked on the
trace analysis and latency experiment as a way to answer that question, but as I
study the code, I really am struggling to justify the complexity that we have
here versus just adding a feature through the chrome stack that tracks latency.
In particular, with some of Antoine's changes to the UpdateRect system, I can't
quite help think that we can make a few basic tweaks in RenderWidgetHost* and
from that have a RenderWidgetHost::getCurrentInputLatency() method [or similar].

I'm not saying these things to block this review. But I do have to say, I'm
concerned about fragility of this test and the amount of trickery its trying to
do to avoid simple additions to the RenderWidget family of functions.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
File chrome/test/data/perf/latency_suite.html (right):

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:4: var frameCountWarmup = 5;
You might want a
/**
 * @fileoverview Blahblahblahparagraph
 */
 at the top of this file that explains to people what the basic structure of
this file is.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:11: var mode = "webgl";
single quotes in js

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:16: var mouse_x = 0;
mouseX, expect_y

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:19: function getParam(key) {
You'd be better served having this return a dictionary of params. E.g. ?a=b&c
reutrns {a: "b", c: true}. Then you could just shovel that into some global
testParams structure and ditch the parseParams and all the defaulting above. If
you want the defaulting, init the dict in here with defaults, then return it...
again, less duping and less globals.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:20: var query =
window.location.href.split('?')[1];
window.location.query.substring(1)?

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:21: if(!query)
On your linux box, run gjslint --strict --check_html on this file. It is
nit-heavy.

if( -> if (

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:38: paintHeavy =
(getParam("paintHeavy") == "true");
So you have to put &paintHeavy=true in the uri? Why not just tweak the query
parsing  to recognize lack of a '=' as defaulting to true? Then you'd just have
getParam("foo")

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:52:
document.getElementById("text").firstChild.nodeValue =
nodeValue -> textContent

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:87: function runTest() {
by the name, i assumed this was called by dom auto. You might want to name this
onLoad.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:94: function delay(milliseconds) {
s/delay/sleep/?

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:97: do { now = new Date(); }
I think you can do this as two lines:

var start = Date.now();
while(Date.now() - start <= milliseconds);

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:101: function xColorString() {
pass in x_ instead and give this a sane name? Or just inline since you're using
this only in the software mode... also, that magic you're doing with substring
is also really funky.... is there some reason for this rather than just (x %
something)?

Fwiw, you can also specify colors as rgb(255,255,255) for example. So, something
I sometimes do is
var g= blah%16;
return 'rgb(' + g + ', ' + g + ', ' + g + ')';

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:150: <table id="table" width="10"
height="10"><tr/></table>
Any reason you're using a table rather than just a plain old div?

For setting sizes for things != canvas (which is funky), its better to  use a
style block, eg
<style>
#table {
  height: 10px;
  width: 10px;
}
</style>

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:152: <canvas id="canvas" width="10"
height="10"></canvas>
why initialize widths here if you initialize it up in code?

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
File chrome/test/perf/rendering/latency_tests.cc (right):

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:74: int kWebGLCanvasWidth = 10;
should these be const? you've got them named as if they're const.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:85:
Query::Phase(TRACE_EVENT_PHASE_INSTANT)),
you might make a note that these will be initialized in runtests... or change
where they init, or something... its a bit confusing that they're all false up
front.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:177: return 16000;
Comment to explain why this magic number is used?

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:199:
rvh->ForwardMouseEvent(mouse_event);
/me wonders out loud whether this will cleanly extend to touch?

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:204: // Print trace data.
comment doesn't add a lot of value methinks

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:249: void LatencyTest::RunTest(const
std::string& mode,
odd that you're using a mode string but an enum for test flags. I'd have
expected a mode enum...

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:250: const std::vector<int>&
behaviors) {
Also, a bit confused that some functions are implemented in the class definition
and some outside. If that's standard, no need to explain, I've not had much
occasion to have a class local to a file before.  :)

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:262: // triggering paints --
otherwise it won't update each frame.
Not sure I follow this. Also, this feels out of place in a block of code that
says its setting up queries.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:308: int mean_frame_time_us =
GetMeanFrameTime();
why are we mixing micros and ms?

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:324: // Run input latency test with
each requested behavior.
This behaviors vs TEST_ distinction reminds me of the frame rate tests issue we
had with "gestures" versus "tests". Where we went was that 1 TEST_ produces 1
number, and we pipe that into the standard perf graphing infrastructure.

Does that learning not apply here for some reason? I would have expected a
different test for every interesting combination of flags rather than 1 test
with ~5 values.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:341: // Print summary if more than 1
behavior was tested in this run.
Is this for human consumption, or for consumption by a log parser? If the
latter, you might want to make that clear...

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:384:
ASSERT_TRUE(message_queue.WaitForMessage(NULL));
Do we need any error handling here? Or is the idea that trace analysis will
miraculuously sort all this out for us?

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:419: printf("Measured latency (in
number of frames) for each frame:\n");
/me is confused about the output of this test. You're printf-ing always? I would
have expected it to pipe into the perf graphing system...

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:420: for (size_t i = 0; i <
events.size(); ++i) {
I'm a little surprise to see an actual loop in user code over the event list.
Does this suggest something about the trace analyzer system that we should
change/adjust?

http://codereview.chromium.org/8883005/diff/18002/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/8883005/diff/18002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host.cc:1039:
UNSHIPPED_TRACE_EVENT_INSTANT2("test_latency", "UpdateRect",
is there any risk of the first byte of the dib being null?

You should probably update the JS to explain that its very important the 0,0'th
pixel has a specific result. It was totally not clear to me that this was the
case, when i was reading it.

http://codereview.chromium.org/8883005/diff/18002/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/8883005/diff/18002/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4142:
UNSHIPPED_TRACE_EVENT_INSTANT1("test_gpu", "DoBlit", "width", srcX1 - srcX0);
Is the idea to use this to connect a main-thread frame to a compositor-thread
frame? I have to say, i still can't quite grok the role of blit in our latency
detection flow...

[jbates, 2011-12-22 02:41:17]

I haven't thought about the scrolling compositor test, but I guess it shouldn't
be too difficult to trace the scroll position and correlate that with the
SwapBuffers to measure it... will have to study this more.

A builtin latency measurement is a good idea. We'll have to discuss how it would
be done. Possibly could work by tracking timestamps on the input events that are
plumbed all the way down to the SwapBuffers and UpdateRects somehow.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
File chrome/test/data/perf/latency_suite.html (right):

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:4: var frameCountWarmup = 5;
On 2011/12/21 20:24:23, nduca wrote:
> You might want a
> /**
>  * @fileoverview Blahblahblahparagraph
>  */
>  at the top of this file that explains to people what the basic structure of
> this file is.

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:11: var mode = "webgl";
On 2011/12/21 20:24:23, nduca wrote:
> single quotes in js

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:16: var mouse_x = 0;
On 2011/12/21 20:24:23, nduca wrote:
> mouseX, expect_y

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:19: function getParam(key) {
On 2011/12/21 20:24:23, nduca wrote:
> You'd be better served having this return a dictionary of params. E.g. ?a=b&c
> reutrns {a: "b", c: true}. Then you could just shovel that into some global
> testParams structure and ditch the parseParams and all the defaulting above.
If
> you want the defaulting, init the dict in here with defaults, then return
it...
> again, less duping and less globals. 

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:20: var query =
window.location.href.split('?')[1];
On 2011/12/21 20:24:23, nduca wrote:
> window.location.query.substring(1)?

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:21: if(!query)
On 2011/12/21 20:24:23, nduca wrote:
> On your linux box, run gjslint --strict --check_html on this file. It is
> nit-heavy.
> 
> if( -> if (

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:38: paintHeavy =
(getParam("paintHeavy") == "true");
On 2011/12/21 20:24:23, nduca wrote:
> So you have to put &paintHeavy=true in the uri? Why not just tweak the query
> parsing  to recognize lack of a '=' as defaulting to true? Then you'd just
have
> getParam("foo")

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:52:
document.getElementById("text").firstChild.nodeValue =
On 2011/12/21 20:24:23, nduca wrote:
> nodeValue -> textContent

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:87: function runTest() {
On 2011/12/21 20:24:23, nduca wrote:
> by the name, i assumed this was called by dom auto. You might want to name
this
> onLoad.

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:94: function delay(milliseconds) {
On 2011/12/21 20:24:23, nduca wrote:
> s/delay/sleep/?

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:97: do { now = new Date(); }
On 2011/12/21 20:24:23, nduca wrote:
> I think you can do this as two lines:
> 
> var start = Date.now();
> while(Date.now() - start <= milliseconds);

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:101: function xColorString() {
On 2011/12/21 20:24:23, nduca wrote:
> pass in x_ instead and give this a sane name? Or just inline since you're
using
> this only in the software mode... also, that magic you're doing with substring
> is also really funky.... is there some reason for this rather than just (x %
> something)?
> 
> Fwiw, you can also specify colors as rgb(255,255,255) for example. So,
something
> I sometimes do is
> var g= blah%16;
> return 'rgb(' + g + ', ' + g + ', ' + g + ')';

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:150: <table id="table" width="10"
height="10"><tr/></table>
On 2011/12/21 20:24:23, nduca wrote:
> Any reason you're using a table rather than just a plain old div?

div didn't maintain its size for some reason, so I went with a table.

> For setting sizes for things != canvas (which is funky), its better to  use a
> style block, eg
> <style>
> #table {
>   height: 10px;
>   width: 10px;
> }
> </style>

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/data/perf/laten...
chrome/test/data/perf/latency_suite.html:152: <canvas id="canvas" width="10"
height="10"></canvas>
On 2011/12/21 20:24:23, nduca wrote:
> why initialize widths here if you initialize it up in code?

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
File chrome/test/perf/rendering/latency_tests.cc (right):

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:74: int kWebGLCanvasWidth = 10;
On 2011/12/21 20:24:23, nduca wrote:
> should these be const? you've got them named as if they're const.

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:85:
Query::Phase(TRACE_EVENT_PHASE_INSTANT)),
On 2011/12/21 20:24:23, nduca wrote:
> you might make a note that these will be initialized in runtests... or change
> where they init, or something... its a bit confusing that they're all false up
> front.

Done. (Sadly no default constructor on Query)

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:177: return 16000;
On 2011/12/21 20:24:23, nduca wrote:
> Comment to explain why this magic number is used?

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:199:
rvh->ForwardMouseEvent(mouse_event);
On 2011/12/21 20:24:23, nduca wrote:
> /me wonders out loud whether this will cleanly extend to touch?

Yeah, it seems like it would be a trivial update to test the "mouse move"
equivalent of touch inputs.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:204: // Print trace data.
On 2011/12/21 20:24:23, nduca wrote:
> comment doesn't add a lot of value methinks

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:249: void LatencyTest::RunTest(const
std::string& mode,
On 2011/12/21 20:24:23, nduca wrote:
> odd that you're using a mode string but an enum for test flags. I'd have
> expected a mode enum...

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:250: const std::vector<int>&
behaviors) {
On 2011/12/21 20:24:23, nduca wrote:
> Also, a bit confused that some functions are implemented in the class
definition
> and some outside. If that's standard, no need to explain, I've not had much
> occasion to have a class local to a file before.  :)

Moved them out (to make the changes easier to follow I did the move as a
separate cl upload from the other changes).

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:262: // triggering paints --
otherwise it won't update each frame.
On 2011/12/21 20:24:23, nduca wrote:
> Not sure I follow this. Also, this feels out of place in a block of code that
> says its setting up queries.

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:308: int mean_frame_time_us =
GetMeanFrameTime();
On 2011/12/21 20:24:23, nduca wrote:
> why are we mixing micros and ms?

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:324: // Run input latency test with
each requested behavior.
On 2011/12/21 20:24:23, nduca wrote:
> This behaviors vs TEST_ distinction reminds me of the frame rate tests issue
we
> had with "gestures" versus "tests". Where we went was that 1 TEST_ produces 1
> number, and we pipe that into the standard perf graphing infrastructure.
> 
> Does that learning not apply here for some reason? I would have expected a
> different test for every interesting combination of flags rather than 1 test
> with ~5 values.

Done.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:341: // Print summary if more than 1
behavior was tested in this run.
On 2011/12/21 20:24:23, nduca wrote:
> Is this for human consumption, or for consumption by a log parser? If the
> latter, you might want to make that clear...

For human consumption, added a comment.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:384:
ASSERT_TRUE(message_queue.WaitForMessage(NULL));
On 2011/12/21 20:24:23, nduca wrote:
> Do we need any error handling here? Or is the idea that trace analysis will
> miraculuously sort all this out for us?

The trace analysis does lots of error checking. Let me know if you see any gaps
in it.

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:419: printf("Measured latency (in
number of frames) for each frame:\n");
On 2011/12/21 20:24:23, nduca wrote:
> /me is confused about the output of this test. You're printf-ing always? I
would
> have expected it to pipe into the perf graphing system...

The only stuff that is parsed by the perf bots comes from the
perf_test::PrintResult* functions. The rest of the printfs are for human
consumption. If a perf test fails on the bots, you can go find the log to see
some basic output from the test and possibly reason about it. The only rule is
that the printf output doesn't look like the perf_test::PrintResult output so as
not to confuse the parser (discussed this with cmp).

http://codereview.chromium.org/8883005/diff/18002/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:420: for (size_t i = 0; i <
events.size(); ++i) {
On 2011/12/21 20:24:23, nduca wrote:
> I'm a little surprise to see an actual loop in user code over the event list.
> Does this suggest something about the trace analyzer system that we should
> change/adjust?

I think it's just the nature of this test. It needs to rewind from specific
events one at a time and count things in between. You'll notice that I added
utility functions FindLastOf, FindClosest and CountMatches just for this test.
Let me know if you see other generic elements of this loop though -- it would be
great to simplify further.

http://codereview.chromium.org/8883005/diff/18002/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/8883005/diff/18002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host.cc:1039:
UNSHIPPED_TRACE_EVENT_INSTANT2("test_latency", "UpdateRect",
On 2011/12/21 20:24:23, nduca wrote:
> is there any risk of the first byte of the dib being null?
> 
This just accesses the first 32bit value of the dib, so it is ok if it's zero.

> You should probably update the JS to explain that its very important the
0,0'th
> pixel has a specific result. It was totally not clear to me that this was the
> case, when i was reading it.

Done.

http://codereview.chromium.org/8883005/diff/18002/gpu/command_buffer/service/...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/8883005/diff/18002/gpu/command_buffer/service/...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4142:
UNSHIPPED_TRACE_EVENT_INSTANT1("test_gpu", "DoBlit", "width", srcX1 - srcX0);
On 2011/12/21 20:24:23, nduca wrote:
> Is the idea to use this to connect a main-thread frame to a compositor-thread
> frame? I have to say, i still can't quite grok the role of blit in our latency
> detection flow...

Yes, exactly as you guessed. The blit represents the compositor consuming a
WebGL frame. This is done when the webgl context has MSAA -- I added a comment
to the html where it creates the context with antialias.

[Paweł Hajdan Jr., 2011-12-22 08:08:39]

http://codereview.chromium.org/8883005/diff/30006/chrome/test/perf/browser_pe...
File chrome/test/perf/browser_perf_test.h (right):

http://codereview.chromium.org/8883005/diff/30006/chrome/test/perf/browser_pe...
chrome/test/perf/browser_perf_test.h:23: void PrintIOPerfInfo(const char*
test_name);
nit: I'm not a fan of "const char*" in the public interface... Could you convert
that to const std::string& ?

My rationale is that there is an implicit conversion from char* to std::string,
and obviously std::string can be passed as std::string, but if you have a
std::string, you have to explicitly use .c_str() when the argument type is
char*, and I don't see anything we'd be gaining by that.

http://codereview.chromium.org/8883005/diff/30006/chrome/test/perf/rendering/...
File chrome/test/perf/rendering/latency_tests.cc (right):

http://codereview.chromium.org/8883005/diff/30006/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:179: // Mode: webgl or software
nit: Dot at the end.

http://codereview.chromium.org/8883005/diff/30006/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:530: CHECK(success);
Are you sure you want to crash the test? This is a browser test so it will
affect only a single test and not the entire test binary, but still if this gets
flaky the test would have to be disabled.

http://codereview.chromium.org/8883005/diff/30006/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:594: #define LATENCY_TEST(prefix,
name, behavior) \
Why a macro? I think gtest has parametrized tests, and I remember we should have
a macro for parametrized IN_PROC_BROWSER_TEST.

http://codereview.chromium.org/8883005/diff/30006/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:614: }  // namespace anonymous
nit: We usually just write "// namespace".

[jbates, 2011-12-22 19:39:34]

http://codereview.chromium.org/8883005/diff/30006/chrome/test/perf/browser_pe...
File chrome/test/perf/browser_perf_test.h (right):

http://codereview.chromium.org/8883005/diff/30006/chrome/test/perf/browser_pe...
chrome/test/perf/browser_perf_test.h:23: void PrintIOPerfInfo(const char*
test_name);
On 2011/12/22 08:08:39, Paweł Hajdan Jr. wrote:
> nit: I'm not a fan of "const char*" in the public interface... Could you
convert
> that to const std::string& ?
> 
> My rationale is that there is an implicit conversion from char* to
std::string,
> and obviously std::string can be passed as std::string, but if you have a
> std::string, you have to explicitly use .c_str() when the argument type is
> char*, and I don't see anything we'd be gaining by that.

Done.

http://codereview.chromium.org/8883005/diff/30006/chrome/test/perf/rendering/...
File chrome/test/perf/rendering/latency_tests.cc (right):

http://codereview.chromium.org/8883005/diff/30006/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:179: // Mode: webgl or software
On 2011/12/22 08:08:39, Paweł Hajdan Jr. wrote:
> nit: Dot at the end.

Done.

http://codereview.chromium.org/8883005/diff/30006/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:530: CHECK(success);
On 2011/12/22 08:08:39, Paweł Hajdan Jr. wrote:
> Are you sure you want to crash the test? This is a browser test so it will
> affect only a single test and not the entire test binary, but still if this
gets
> flaky the test would have to be disabled.

Done. (Changed to void method with ASSERT_TRUE.)

http://codereview.chromium.org/8883005/diff/30006/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:594: #define LATENCY_TEST(prefix,
name, behavior) \
On 2011/12/22 08:08:39, Paweł Hajdan Jr. wrote:
> Why a macro? I think gtest has parametrized tests, and I remember we should
have
> a macro for parametrized IN_PROC_BROWSER_TEST.

Done.

http://codereview.chromium.org/8883005/diff/30006/chrome/test/perf/rendering/...
chrome/test/perf/rendering/latency_tests.cc:614: }  // namespace anonymous
On 2011/12/22 08:08:39, Paweł Hajdan Jr. wrote:
> nit: We usually just write "// namespace".

Done.

[Paweł Hajdan Jr., 2011-12-23 09:19:22]

LGTM

[nduca, 2012-01-03 18:51:55]

LGTM

[commit-bot: I haz the power, 2012-01-03 19:46:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbates@chromium.org/8883005/37010

[commit-bot: I haz the power, 2012-01-03 21:02:02]

Change committed as 116191