Give performance_browser_tests lots of love and attention.

Give performance_browser_tests lots of love and attention.

This change started by fixing a bug where a trace event was missing,
causing failures on the Win7 bots. It quickly evolved into a number of
improvements, focusing on measurement stability, and adding a few
additional useful analyses. More-specifically:

1. The logic that tracks a frame in the CastV2PerformanceTest was
simplified since exact timestamps are now available in the tracing data
(i.e., no need for fabs(TS_a - TS_b) < 1 millisecond" searches for prior
events).

2. Trimmed events at both the start and end of the tests, and updated
the number of trimmed events, to promote more stable measurements in the
perf waterfall. Both CastV2PerformanceTests and
TabCapturePerformanceTests now run for 15 seconds (before, this was 20
and 10, respectively).

3. Added new "frame_drop_rate" measurement to CastV2PerformanceTest.
Similarly, added a CaptureFailRate measurement to
TabCapturePerformanceTest.

4. Added performance testing with auto throttling (aka "zero config")
enabled to CastV2PerformanceTest, through simulated "slow, but reliable
Wifi." If measurements are stable, we can add monitoring for the average
capture resolution (larger is better) and number of resolution changes
(lower is better) as way to detect perf regressions that will have a
direct impact on the user experience.

5. Enabled encryption on the Cast Streaming transport, to ensure crypto
is also being considered when measuring performance and alerting on
regressions.

6. Minor code clean-up: style stuff, de-duping utility functions, fixing
use of the deprecated MediaStream.stop() call, adding/clarifying
comments to account for changes over the past couple of years.

Added results:
  CastV2Performance*: playout_resolution
  CastV2Performance*: resolution_changes
  CastV2Performance*: frame_drop_rate
  TabCapturePerformance*: CaptureLatency
  TabCapturePerformance*: CaptureFailRate

Removed results:
  TabCapturePerformance*: CaptureSucceeded

BUG=567848, 709247
Review-Url: https://codereview.chromium.org/2866943002
Cr-Commit-Position: refs/heads/master@{#474189}
Committed: https://chromium.googlesource.com/chromium/src/+/50ca2ed80efff90dcc3192c1e3e1c2d782d14a91

[miu, 2017-05-06 21:57:46]

Patchset #1 (id:1) has been deleted

[miu, 2017-05-06 22:34:04]

The CQ bit was checked by miu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-06 22:34:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[miu, 2017-05-06 22:36:06]

miu@chromium.org changed reviewers:
+ hubbe@chromium.org

[miu, 2017-05-06 22:36:07]

hubbe: Mind taking a look, given you're most-familiar with all the code I'm
changing?

[commit-bot: I haz the power, 2017-05-06 22:37:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-06 22:37:25]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[miu, 2017-05-06 22:43:31]

The CQ bit was checked by miu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-06 22:43:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-07 00:07:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-07 00:07:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hubbe, 2017-05-07 07:24:52]

On 2017/05/06 22:36:07, miu wrote:
> hubbe: Mind taking a look, given you're most-familiar with all the code I'm
> changing?

Ow, that's a lot of changes for one CL...
I'll try to review on Monday, but things will go faster it was split up into
incremental CLs.

[miu, 2017-05-23 07:38:44]

miu@chromium.org changed reviewers:
+ xjz@chromium.org
- hubbe@chromium.org

[miu, 2017-05-23 07:38:45]

xjz: PTAL. Seems hubbe@'s got too much on his plate right now.

[xjz, 2017-05-23 20:51:01]

lgtm

https://codereview.chromium.org/2866943002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/cast_streaming/performance_test.cc (right):

https://codereview.chromium.org/2866943002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_streaming/performance_test.cc:232: for
(size_t i = kTrimEvents; i < events.size(); i++) {
nit: Do we want to trim the events from the end too?

https://codereview.chromium.org/2866943002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/tab_capture/tab_capture_performancetest.cc
(right):

https://codereview.chromium.org/2866943002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/tab_capture_performancetest.cc:189: }
nit: DCHECK_GE(count, kMinDataPoints)?

[hubbe, 2017-05-23 21:26:27]

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

[hubbe, 2017-05-23 21:26:28]

https://codereview.chromium.org/2866943002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/cast_streaming/performance_test.cc (right):

https://codereview.chromium.org/2866943002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_streaming/performance_test.cc:110: // If
testing with a receiver clock that is ahead or behind the sender
I don't think this really works, since PostDelayTask won't honor the altered
rate.

https://codereview.chromium.org/2866943002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_streaming/performance_test.cc:112: // per
million faster or slower than the local sender's clock. This is the
50 ppm might be the worst we encounter in RL, but it's not enough to trigger any
problems in a short test. I suggest making it 25% or something instead.

https://codereview.chromium.org/2866943002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_streaming/performance_test.cc:286: // Number
of resolution changes. Lower is better (and 1 is ideal). Zero
It's probably better to print out nothing if there is no data.

[miu, 2017-05-23 21:51:05]

https://codereview.chromium.org/2866943002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/cast_streaming/performance_test.cc (right):

https://codereview.chromium.org/2866943002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_streaming/performance_test.cc:232: for
(size_t i = kTrimEvents; i < events.size(); i++) {
On 2017/05/23 20:51:01, xjz wrote:
> nit: Do we want to trim the events from the end too? 

Done.

https://codereview.chromium.org/2866943002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/tab_capture/tab_capture_performancetest.cc
(right):

https://codereview.chromium.org/2866943002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/tab_capture_performancetest.cc:189: }
On 2017/05/23 20:51:01, xjz wrote:
> nit: DCHECK_GE(count, kMinDataPoints)?

Done.

[miu, 2017-05-23 21:51:11]

The CQ bit was checked by miu@chromium.org

[miu, 2017-05-23 21:51:12]

The patchset sent to the CQ was uploaded after l-g-t-m from xjz@chromium.org
Link to the patchset: https://codereview.chromium.org/2866943002/#ps80001
(title: "Fixed things xjz@ mentioned.")

[commit-bot: I haz the power, 2017-05-23 21:52:04]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[miu, 2017-05-23 22:09:22]

Thanks, hubbe. I just noticed your comments right after I started the commit.
I've considered your points, but given the urgency for getting the tests
up-and-running again, I'm going to keep this in the commit queue. I'll follow-up
on the "skewed clock" issues in a later change.

https://codereview.chromium.org/2866943002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/cast_streaming/performance_test.cc (right):

https://codereview.chromium.org/2866943002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_streaming/performance_test.cc:110: // If
testing with a receiver clock that is ahead or behind the sender
On 2017/05/23 21:26:28, hubbe wrote:
> I don't think this really works, since PostDelayTask won't honor the altered
> rate.

It sort of does: Code will use Now() to determine how much delay to give to
PostDelayedTask. But, you're right in that PostDelayedTask won't be using the
skewed clock as a timer. Hmm...I'll need to reconsider this.

https://codereview.chromium.org/2866943002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_streaming/performance_test.cc:112: // per
million faster or slower than the local sender's clock. This is the
On 2017/05/23 21:26:28, hubbe wrote:
> 50 ppm might be the worst we encounter in RL, but it's not enough to trigger
any
> problems in a short test. I suggest making it 25% or something instead.

Guidance from the perf folks is not to test things that are unlikely to happen
in RL. Maybe I should consider just dropping the skewed clock altogether?

https://codereview.chromium.org/2866943002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_streaming/performance_test.cc:286: // Number
of resolution changes. Lower is better (and 1 is ideal). Zero
On 2017/05/23 21:26:28, hubbe wrote:
> It's probably better to print out nothing if there is no data.

The EXPECT_GT() a few lines down should fail in this case, which will turn the
bots red. Or, are you saying there is a more-preferred "standard practice" I
should be following here?

[commit-bot: I haz the power, 2017-05-23 22:36:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-23 22:36:04]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hubbe, 2017-05-23 22:47:08]

https://codereview.chromium.org/2866943002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/cast_streaming/performance_test.cc (right):

https://codereview.chromium.org/2866943002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_streaming/performance_test.cc:112: // per
million faster or slower than the local sender's clock. This is the
On 2017/05/23 22:09:22, miu wrote:
> On 2017/05/23 21:26:28, hubbe wrote:
> > 50 ppm might be the worst we encounter in RL, but it's not enough to trigger
> any
> > problems in a short test. I suggest making it 25% or something instead.
> 
> Guidance from the perf folks is not to test things that are unlikely to happen
> in RL. Maybe I should consider just dropping the skewed clock altogether?

Probably better to drop it.
I think we have some other tests that tests what happens when the clock is fast
or slow, so i don't think we need to have it in the end-to-end tests.

https://codereview.chromium.org/2866943002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_streaming/performance_test.cc:286: // Number
of resolution changes. Lower is better (and 1 is ideal). Zero
On 2017/05/23 22:09:22, miu wrote:
> On 2017/05/23 21:26:28, hubbe wrote:
> > It's probably better to print out nothing if there is no data.
> 
> The EXPECT_GT() a few lines down should fail in this case, which will turn the
> bots red. Or, are you saying there is a more-preferred "standard practice" I
> should be following here?

EXPECT_GT should be fine.

[miu, 2017-05-24 00:00:33]

The CQ bit was checked by miu@chromium.org

[miu, 2017-05-24 00:00:34]

The patchset sent to the CQ was uploaded after l-g-t-m from xjz@chromium.org
Link to the patchset: https://codereview.chromium.org/2866943002/#ps100001
(title: "fix compile error")

[commit-bot: I haz the power, 2017-05-24 00:01:27]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[miu, 2017-05-24 02:27:59]

Patchset #6 (id:120001) has been deleted

[miu, 2017-05-24 02:29:37]

The CQ bit was checked by miu@chromium.org

[miu, 2017-05-24 02:29:38]

The patchset sent to the CQ was uploaded after l-g-t-m from xjz@chromium.org
Link to the patchset: https://codereview.chromium.org/2866943002/#ps100001
(title: "fix compile error")

[commit-bot: I haz the power, 2017-05-24 02:31:19]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 06:55:56]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495592977885500,
"parent_rev": "fb83c2d4d2044b788c5fe8ebe35937413c2d72ba", "commit_rev":
"50ca2ed80efff90dcc3192c1e3e1c2d782d14a91"}

[commit-bot: I haz the power, 2017-05-24 06:56:09]

Description was changed from

==========
Give performance_browser_tests lots of love and attention.

This change started by fixing a bug where a trace event was missing,
causing failures on the Win7 bots. It quickly evolved into a number of
improvements, focusing on measurement stability, and adding a few
additional useful analyses. More-specifically:

1. The logic that tracks a frame in the CastV2PerformanceTest was
simplified since exact timestamps are now available in the tracing data
(i.e., no need for fabs(TS_a - TS_b) < 1 millisecond" searches for prior
events).

2. Trimmed events at both the start and end of the tests, and updated
the number of trimmed events, to promote more stable measurements in the
perf waterfall. Both CastV2PerformanceTests and
TabCapturePerformanceTests now run for 15 seconds (before, this was 20
and 10, respectively).

3. Added new "frame_drop_rate" measurement to CastV2PerformanceTest.
Similarly, added a CaptureFailRate measurement to
TabCapturePerformanceTest.

4. Added performance testing with auto throttling (aka "zero config")
enabled to CastV2PerformanceTest, through simulated "slow, but reliable
Wifi." If measurements are stable, we can add monitoring for the average
capture resolution (larger is better) and number of resolution changes
(lower is better) as way to detect perf regressions that will have a
direct impact on the user experience.

5. Enabled encryption on the Cast Streaming transport, to ensure crypto
is also being considered when measuring performance and alerting on
regressions.

6. Minor code clean-up: style stuff, de-duping utility functions, fixing
use of the deprecated MediaStream.stop() call, adding/clarifying
comments to account for changes over the past couple of years.

Added results:
  CastV2Performance*: playout_resolution
  CastV2Performance*: resolution_changes
  CastV2Performance*: frame_drop_rate
  TabCapturePerformance*: CaptureLatency
  TabCapturePerformance*: CaptureFailRate

Removed results:
  TabCapturePerformance*: CaptureSucceeded

BUG=567848,709247
==========

to

==========
Give performance_browser_tests lots of love and attention.

This change started by fixing a bug where a trace event was missing,
causing failures on the Win7 bots. It quickly evolved into a number of
improvements, focusing on measurement stability, and adding a few
additional useful analyses. More-specifically:

1. The logic that tracks a frame in the CastV2PerformanceTest was
simplified since exact timestamps are now available in the tracing data
(i.e., no need for fabs(TS_a - TS_b) < 1 millisecond" searches for prior
events).

2. Trimmed events at both the start and end of the tests, and updated
the number of trimmed events, to promote more stable measurements in the
perf waterfall. Both CastV2PerformanceTests and
TabCapturePerformanceTests now run for 15 seconds (before, this was 20
and 10, respectively).

3. Added new "frame_drop_rate" measurement to CastV2PerformanceTest.
Similarly, added a CaptureFailRate measurement to
TabCapturePerformanceTest.

4. Added performance testing with auto throttling (aka "zero config")
enabled to CastV2PerformanceTest, through simulated "slow, but reliable
Wifi." If measurements are stable, we can add monitoring for the average
capture resolution (larger is better) and number of resolution changes
(lower is better) as way to detect perf regressions that will have a
direct impact on the user experience.

5. Enabled encryption on the Cast Streaming transport, to ensure crypto
is also being considered when measuring performance and alerting on
regressions.

6. Minor code clean-up: style stuff, de-duping utility functions, fixing
use of the deprecated MediaStream.stop() call, adding/clarifying
comments to account for changes over the past couple of years.

Added results:
  CastV2Performance*: playout_resolution
  CastV2Performance*: resolution_changes
  CastV2Performance*: frame_drop_rate
  TabCapturePerformance*: CaptureLatency
  TabCapturePerformance*: CaptureFailRate

Removed results:
  TabCapturePerformance*: CaptureSucceeded

BUG=567848,709247

Review-Url: https://codereview.chromium.org/2866943002
Cr-Commit-Position: refs/heads/master@{#474189}
Committed:
https://chromium.googlesource.com/chromium/src/+/50ca2ed80efff90dcc3192c1e3e1...
==========

[commit-bot: I haz the power, 2017-05-24 06:56:11]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/50ca2ed80efff90dcc3192c1e3e1...