Performance measures of old and new RTCPeerConnection.getStats added.

Performance measures of old and new RTCPeerConnection.getStats added.

The following Chrome Performance Dashboard[1] stats are added:
- browser_tests / getStats_[promise/callback] / invocation_time (ms)

[1] https://chromeperf.appspot.com/

BUG=chromium:627816
Review-Url: https://codereview.chromium.org/2641263003
Cr-Commit-Position: refs/heads/master@{#446668}
Committed: https://chromium.googlesource.com/chromium/src/+/98b3081a01af68384eea9d82f169079162a1edde

[hbos_chromium, 2017-01-20 13:36:04]

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

[commit-bot: I haz the power, 2017-01-20 13:36:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hbos_chromium, 2017-01-20 13:59:08]

hbos@chromium.org changed reviewers:
+ hta@chromium.org, tommi@chromium.org

[hbos_chromium, 2017-01-20 13:59:09]

Please take a look, hta and tommi.

Example measurements on local machine:

RESULT getStats_callback: invocation_time= 17.907 milliseconds
RESULT getStats_callback: invocation_time= 22.211 milliseconds
RESULT getStats_callback: invocation_time= 23.482 milliseconds
RESULT getStats_callback: invocation_time= 24.909 milliseconds
RESULT getStats_callback: invocation_time= 25.135 milliseconds
Average for callback-based (old) getStats: 22.7288 milliseconds

RESULT getStats_promise: invocation_time= 17.204 milliseconds
RESULT getStats_promise: invocation_time= 18.119 milliseconds
RESULT getStats_promise: invocation_time= 18.897 milliseconds
RESULT getStats_promise: invocation_time= 19.068 milliseconds
RESULT getStats_promise: invocation_time= 21.185 milliseconds
Average for promise-based (new) getStats: 18.8946 milliseconds

Difference in average: -3.8342 ms (16.87 % faster)

[commit-bot: I haz the power, 2017-01-20 14:04:44]

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

[commit-bot: I haz the power, 2017-01-20 14:04:45]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tommi (sloooow) - chröme, 2017-01-20 19:31:00]

Are the results comparing gathering of the same stats?

On Fri, Jan 20, 2017, 13:59 <hbos@chromium.org> wrote:

> Reviewers: hta - Chromium, tommi (chrømium) - ooo 18-19th
> CL: https://codereview.chromium.org/2641263003/
>
> Message:
> Please take a look, hta and tommi.
>
> Example measurements on local machine:
>
> RESULT getStats_callback: invocation_time= 17.907 milliseconds
> RESULT getStats_callback: invocation_time= 22.211 milliseconds
> RESULT getStats_callback: invocation_time= 23.482 milliseconds
> RESULT getStats_callback: invocation_time= 24.909 milliseconds
> RESULT getStats_callback: invocation_time= 25.135 milliseconds
> Average for callback-based (old) getStats: 22.7288 milliseconds
>
> RESULT getStats_promise: invocation_time= 17.204 milliseconds
> RESULT getStats_promise: invocation_time= 18.119 milliseconds
> RESULT getStats_promise: invocation_time= 18.897 milliseconds
> RESULT getStats_promise: invocation_time= 19.068 milliseconds
> RESULT getStats_promise: invocation_time= 21.185 milliseconds
> Average for promise-based (new) getStats: 18.8946 milliseconds
>
> Difference in average: -3.8342 ms (16.87 % faster)
>
> Description:
> Performance measures of old and new RTCPeerConnection.getStats added.
>
> The following Chrome Performance Dashboard[1] stats are added:
> - browser_tests / getStats_[promise/callback] / invocation_time (ms)
>
> [1] https://chromeperf.appspot.com/
>
> BUG=chromium:627816
>
> Affected files (+166, -52 lines):
> M chrome/browser/media/webrtc/webrtc_browsertest_base.h
> M chrome/browser/media/webrtc/webrtc_browsertest_base.cc
> M chrome/browser/media/webrtc/webrtc_stats_perf_browsertest.cc
> M chrome/test/data/webrtc/peerconnection.js
> M chrome/test/data/webrtc/peerconnection_getstats.js
>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hbos_chromium, 2017-01-23 10:44:30]

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

[commit-bot: I haz the power, 2017-01-23 10:44:46]

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-01-23 11:42:48]

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

[commit-bot: I haz the power, 2017-01-23 11:42:49]

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

[hbos_chromium, 2017-01-23 11:52:30]

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

[commit-bot: I haz the power, 2017-01-23 11:52:39]

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-01-23 12:38:59]

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

[commit-bot: I haz the power, 2017-01-23 12:39:00]

Dry run: This issue passed the CQ dry run.

[hbos_chromium, 2017-01-23 13:00:19]

On 2017/01/20 19:31:00, tommi (krómíum) wrote:
> Are the results comparing gathering of the same stats?

This is the old and new API for getting *all* supported stats. They are not the
same set of stats because the spec (new stats) is very different from the old
stats.
But there is overlap (see doc I shared with you), and with everything wired up
like it is now, if we want to add old stats to the new stats it's just a matter
of performing a '=' operation per stat in most cases and is not expected to
noticeably affect the performance.

[hbos_chromium, 2017-01-23 13:56:00]

Please take a look, tommi and hta.

[tommi (sloooow) - chröme, 2017-01-23 14:19:52]

lgtm

https://codereview.chromium.org/2641263003/diff/20001/chrome/browser/media/we...
File chrome/browser/media/webrtc/webrtc_stats_perf_browsertest.cc (right):

https://codereview.chromium.org/2641263003/diff/20001/chrome/browser/media/we...
chrome/browser/media/webrtc/webrtc_stats_perf_browsertest.cc:118: if (left_tab_)
{
nit: no need for {} in cases like this

[hta - Chromium, 2017-01-23 14:53:52]

Looks good, but I'd like you to answer a couple of questions (in comments in
code, preferably).

And of course there's a rename I want...

https://codereview.chromium.org/2641263003/diff/20001/chrome/browser/media/we...
File chrome/browser/media/webrtc/webrtc_stats_perf_browsertest.cc (right):

https://codereview.chromium.org/2641263003/diff/20001/chrome/browser/media/we...
chrome/browser/media/webrtc/webrtc_stats_perf_browsertest.cc:27: enum class
GetStatsVariety {
English nit: This should be "GetStatsVariation", not "Variety".

(I think the term for "variety" is a collective noun - meaning a set of things
that have differences. While "variation" is a single thing that is different
from something else.)

https://codereview.chromium.org/2641263003/diff/20001/chrome/browser/media/we...
chrome/browser/media/webrtc/webrtc_stats_perf_browsertest.cc:202:
EXPECT_TRUE(base::TimeTicks::IsHighResolution());
From the peanut gallery: I see that you are using an "execute this Javascript"
procedure. Do you have a number for what the overhead of a do-nothing "execute
in Javascript" call is?

https://codereview.chromium.org/2641263003/diff/20001/chrome/browser/media/we...
chrome/browser/media/webrtc/webrtc_stats_perf_browsertest.cc:208:
test::SleepInJavascript(left_tab_, 10000);
Have you done some subjective testing to see if the test is actually affected by
this timer? 10 seconds sounds like a long time, and given the number of bots we
run, those seconds add up.

I'd suggest bisecting the space between 0 and 10 seconds in manual runs, find
the place where performance starts to change or become unstable, and pick a
number that's double that.
And then add a comment saying something like

const wait_length = 10000
// 10 seconds. We've seen performance vary if we only wait 5 seconds.

https://codereview.chromium.org/2641263003/diff/20001/chrome/browser/media/we...
chrome/browser/media/webrtc/webrtc_stats_perf_browsertest.cc:213:
GetStatsAndReturn(left_tab_);
Won't you get a semi-independent measure of the same thing if you also do
GetStatsAndReturn(right_tab_)?

[hbos_chromium, 2017-01-24 12:07:27]

Patchset #3 (id:40001) has been deleted

[hbos_chromium, 2017-01-24 12:07:52]

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

[commit-bot: I haz the power, 2017-01-24 12:08:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hbos_chromium, 2017-01-24 12:08:37]

PTAL hta.

Removing the overhead of C++ calling JavaScript changed the numbers quite a bit,
here are new measurements:

RESULT getStats_callback: invocation_time= 9.682500000000118 milliseconds
RESULT getStats_callback: invocation_time= 10.715000000000373 milliseconds
RESULT getStats_callback: invocation_time= 10.850000000000364 milliseconds
RESULT getStats_callback: invocation_time= 11.154999999999518 milliseconds
RESULT getStats_callback: invocation_time= 11.305000000000291 milliseconds

Average: 10.7415 ms

RESULT getStats_promise: invocation_time= 6.869999999999436 milliseconds
RESULT getStats_promise: invocation_time= 7.367500000000064 milliseconds
RESULT getStats_promise: invocation_time= 7.5949999999998 milliseconds
RESULT getStats_promise: invocation_time= 9.637499999999818 milliseconds
RESULT getStats_promise: invocation_time= 9.9849999999999 milliseconds

Average: 8.291 ms (22.81% faster)

https://codereview.chromium.org/2641263003/diff/20001/chrome/browser/media/we...
File chrome/browser/media/webrtc/webrtc_stats_perf_browsertest.cc (right):

https://codereview.chromium.org/2641263003/diff/20001/chrome/browser/media/we...
chrome/browser/media/webrtc/webrtc_stats_perf_browsertest.cc:27: enum class
GetStatsVariety {
On 2017/01/23 14:53:52, hta - Chromium wrote:
> English nit: This should be "GetStatsVariation", not "Variety".
> 
> (I think the term for "variety" is a collective noun - meaning a set of things
> that have differences. While "variation" is a single thing that is different
> from something else.)

Done.

https://codereview.chromium.org/2641263003/diff/20001/chrome/browser/media/we...
chrome/browser/media/webrtc/webrtc_stats_perf_browsertest.cc:118: if (left_tab_)
{
On 2017/01/23 14:19:51, tommi (krómíum) wrote:
> nit: no need for {} in cases like this

Done.

https://codereview.chromium.org/2641263003/diff/20001/chrome/browser/media/we...
chrome/browser/media/webrtc/webrtc_stats_perf_browsertest.cc:202:
EXPECT_TRUE(base::TimeTicks::IsHighResolution());
On 2017/01/23 14:53:52, hta - Chromium wrote:
> From the peanut gallery: I see that you are using an "execute this Javascript"
> procedure. Do you have a number for what the overhead of a do-nothing "execute
> in Javascript" call is?

I moved the time measurements to the JS layer. This halved the performance
measurements, with 10+ ms overhead to "call javascript function from c++".

https://codereview.chromium.org/2641263003/diff/20001/chrome/browser/media/we...
chrome/browser/media/webrtc/webrtc_stats_perf_browsertest.cc:208:
test::SleepInJavascript(left_tab_, 10000);
On 2017/01/23 14:53:52, hta - Chromium wrote:
> Have you done some subjective testing to see if the test is actually affected
by
> this timer? 10 seconds sounds like a long time, and given the number of bots
we
> run, those seconds add up.
> 
> I'd suggest bisecting the space between 0 and 10 seconds in manual runs, find
> the place where performance starts to change or become unstable, and pick a
> number that's double that.
> And then add a comment saying something like
> 
> const wait_length = 10000
> // 10 seconds. We've seen performance vary if we only wait 5 seconds.
> 

The idea was to get full coverage of stats, things like echo cancellation might
not have a value immediately, but this doesn't really matter. With manual
testing showing a high variation regardless of sleep time I removed the sleep
call.

https://codereview.chromium.org/2641263003/diff/20001/chrome/browser/media/we...
chrome/browser/media/webrtc/webrtc_stats_perf_browsertest.cc:213:
GetStatsAndReturn(left_tab_);
On 2017/01/23 14:53:52, hta - Chromium wrote:
> Won't you get a semi-independent measure of the same thing if you also do
> GetStatsAndReturn(right_tab_)?
> 

Done, taking average of left and right tab's getStats.

[commit-bot: I haz the power, 2017-01-24 12:37:05]

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

[commit-bot: I haz the power, 2017-01-24 12:37:05]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[hbos_chromium, 2017-01-24 12:41:21]

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

[commit-bot: I haz the power, 2017-01-24 12:41:37]

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-01-24 13:13:18]

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

[commit-bot: I haz the power, 2017-01-24 13:13:19]

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

[hta - Chromium, 2017-01-26 08:13:58]

lgtm

https://codereview.chromium.org/2641263003/diff/80001/chrome/browser/media/we...
File chrome/browser/media/webrtc/webrtc_stats_perf_browsertest.cc (right):

https://codereview.chromium.org/2641263003/diff/80001/chrome/browser/media/we...
chrome/browser/media/webrtc/webrtc_stats_perf_browsertest.cc:111:
CreateDataChannel(right_tab_, "data");
I appreciate that creating data channels gives more stats to collect, but will
it have any effect on any of the other tests that use this?

(I'm assuming answer is "no, not really", since they never use it).

[hbos_chromium, 2017-01-27 14:07:57]

https://codereview.chromium.org/2641263003/diff/80001/chrome/browser/media/we...
File chrome/browser/media/webrtc/webrtc_stats_perf_browsertest.cc (right):

https://codereview.chromium.org/2641263003/diff/80001/chrome/browser/media/we...
chrome/browser/media/webrtc/webrtc_stats_perf_browsertest.cc:111:
CreateDataChannel(right_tab_, "data");
On 2017/01/26 08:13:58, hta - Chromium wrote:
> I appreciate that creating data channels gives more stats to collect, but will
> it have any effect on any of the other tests that use this?
> 
> (I'm assuming answer is "no, not really", since they never use it).

Nah, doesn't matter. I could add an argument to specify if we want data channels
but there's no benefit to not adding them. Having a complete set of stats just
means more code coverage and easier to update the test if we want to inspect
anything data channels related in the future.

[hbos_chromium, 2017-01-27 14:08:26]

The CQ bit was checked by hbos@chromium.org

[hbos_chromium, 2017-01-27 14:08:26]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org
Link to the patchset: https://codereview.chromium.org/2641263003/#ps80001
(title: "Avoiding win uninitialized variable warning")

[commit-bot: I haz the power, 2017-01-27 14:08:42]

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-01-27 15:12:24]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485526106573770,
"parent_rev": "265ae0467148c0872ccae6c862813d27f21ce031", "commit_rev":
"98b3081a01af68384eea9d82f169079162a1edde"}

[commit-bot: I haz the power, 2017-01-27 15:12:57]

Description was changed from

==========
Performance measures of old and new RTCPeerConnection.getStats added.

The following Chrome Performance Dashboard[1] stats are added:
- browser_tests / getStats_[promise/callback] / invocation_time (ms)

[1] https://chromeperf.appspot.com/

BUG=chromium:627816
==========

to

==========
Performance measures of old and new RTCPeerConnection.getStats added.

The following Chrome Performance Dashboard[1] stats are added:
- browser_tests / getStats_[promise/callback] / invocation_time (ms)

[1] https://chromeperf.appspot.com/

BUG=chromium:627816

Review-Url: https://codereview.chromium.org/2641263003
Cr-Commit-Position: refs/heads/master@{#446668}
Committed:
https://chromium.googlesource.com/chromium/src/+/98b3081a01af68384eea9d82f169...
==========

[commit-bot: I haz the power, 2017-01-27 15:12:58]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/98b3081a01af68384eea9d82f169...