added WebRTC-QuickPerfTest to RampUpTests and CallPerfTests

added WebRTC-QuickPerfTest to RampUpTests and CallPerfTests

BUG=webrtc:7153
Review-Url: https://codereview.webrtc.org/2708723002
Cr-Commit-Position: refs/heads/master@{#16743}
Committed: https://chromium.googlesource.com/external/webrtc/+/5328b9eb328e049cb19c5b3ce99fdbf2bbae25e5

[ilnik, 2017-02-20 16:04:40]

ilnik@webrtc.org changed reviewers:
+ stefan@webrtc.org

[ilnik, 2017-02-20 16:04:40]

Not sure if these truncated tests are even checking anything sensible, but some
work is being done, some checks pass. If timeout is set to 300ms some tests
fail. Currently I set it to 500ms. If tests will be flaky we could just increase
kShortTimeoutMs.

[stefan-webrtc, 2017-02-20 16:16:20]

On 2017/02/20 16:04:40, ilnik wrote:
> Not sure if these truncated tests are even checking anything sensible, but
some
> work is being done, some checks pass. If timeout is set to 300ms some tests
> fail. Currently I set it to 500ms. If tests will be flaky we could just
increase
> kShortTimeoutMs.

I don't think they do much, but perhaps it's still worth running them...

[stefan-webrtc, 2017-02-20 16:16:24]

lgtm

[stefan-webrtc, 2017-02-20 16:37:01]

not lgtm

I missed the fact that this affects videosendstream tests and end to end tests.
I think we should try to avoid that, as it's important that we run the real
versions of those on try bots.

[ilnik, 2017-02-21 08:53:46]

On 2017/02/20 16:37:01, stefan-webrtc wrote:
> not lgtm
> 
> I missed the fact that this affects videosendstream tests and end to end
tests.
> I think we should try to avoid that, as it's important that we run the real
> versions of those on try bots.

But VideoSendStream and EndToEndTest are linked in webrtc_tests, not
webrtc_perf_tests. Will it be possible to pass
--force_fieldtrials=WebRTC-QuickPerfTest/Enabled/ only to webrtc_perf_tests
executable? That way only perf tests will be affected.

[ilnik, 2017-02-21 09:07:37]

On 2017/02/20 16:37:01, stefan-webrtc wrote:
> not lgtm
> 
> I missed the fact that this affects videosendstream tests and end to end
tests.
> I think we should try to avoid that, as it's important that we run the real
> versions of those on try bots.

Actually, kjellander@ confirmed, that flag will be passed only to
webrtc_perf_tests. So VideoSendStream and EndToEnd tests are not affected. 
I am still working on VideoSendersTest to speed it up. After that new LGTM will
be needed.

[ilnik, 2017-02-21 09:30:04]

On 2017/02/21 09:07:37, ilnik wrote:
> On 2017/02/20 16:37:01, stefan-webrtc wrote:
> > not lgtm
> > 
> > I missed the fact that this affects videosendstream tests and end to end
> tests.
> > I think we should try to avoid that, as it's important that we run the real
> > versions of those on try bots.
> 
> Actually, kjellander@ confirmed, that flag will be passed only to
> webrtc_perf_tests. So VideoSendStream and EndToEnd tests are not affected. 
> I am still working on VideoSendersTest to speed it up. After that new LGTM
will
> be needed.

Now VideoSenderTest and NetEqPerformanceTest are also speed-up. Produced metrics
make no sense, but they will be ignored anyways. That's important is that tests
pass, even though some work is done. Total runtime of webrtc_perf_tests is now
reduced to 46s.

[stefan-webrtc, 2017-02-21 09:35:45]

stefan@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org

[stefan-webrtc, 2017-02-21 09:35:46]

Thanks for clarifying.

Adding hlundin for neteq.

https://codereview.webrtc.org/2708723002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/neteq_performance_test.cc (right):

https://codereview.webrtc.org/2708723002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_performance_test.cc:31: const int
kQuickTestTimeoutMs = 100000;
Sounds like a lot to have 100 seconds for a quick test run?

[ilnik, 2017-02-21 09:46:08]

https://codereview.webrtc.org/2708723002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/neteq_performance_test.cc (right):

https://codereview.webrtc.org/2708723002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_performance_test.cc:31: const int
kQuickTestTimeoutMs = 100000;
On 2017/02/21 09:35:46, stefan-webrtc wrote:
> Sounds like a lot to have 100 seconds for a quick test run?

The variable is named runtime_ms, but it is not a realtime duration, but
simulated runtime. No clocks are tied to it. It turns out, simulation is ~300
times faster than realtime. Originally, tests are run with 10 million ms there.
This value of 100000 is already 100 times less than that.

[stefan-webrtc, 2017-02-21 10:26:03]

https://codereview.webrtc.org/2708723002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/neteq_performance_test.cc (right):

https://codereview.webrtc.org/2708723002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_performance_test.cc:31: const int
kQuickTestTimeoutMs = 100000;
On 2017/02/21 09:46:08, ilnik wrote:
> On 2017/02/21 09:35:46, stefan-webrtc wrote:
> > Sounds like a lot to have 100 seconds for a quick test run?
> 
> The variable is named runtime_ms, but it is not a realtime duration, but
> simulated runtime. No clocks are tied to it. It turns out, simulation is ~300
> times faster than realtime. Originally, tests are run with 10 million ms
there.
> This value of 100000 is already 100 times less than that.

Ok

I would prefer if you moved this to where the Run() method is being called, so
you can do:

Run(field_trial::FindFullName("WebRTC-QuickPerfTest") == "Enabled" ?
kQuickTestTimeoutMs : kRegularTestTimeoutMs, loss_rate, drift_factor);

That way the caller won't be surprised.

[ilnik, 2017-02-21 10:28:11]

On 2017/02/21 10:26:03, stefan-webrtc wrote:
>
https://codereview.webrtc.org/2708723002/diff/20001/webrtc/modules/audio_codi...
> File webrtc/modules/audio_coding/neteq/tools/neteq_performance_test.cc
(right):
> 
>
https://codereview.webrtc.org/2708723002/diff/20001/webrtc/modules/audio_codi...
> webrtc/modules/audio_coding/neteq/tools/neteq_performance_test.cc:31: const
int
> kQuickTestTimeoutMs = 100000;
> On 2017/02/21 09:46:08, ilnik wrote:
> > On 2017/02/21 09:35:46, stefan-webrtc wrote:
> > > Sounds like a lot to have 100 seconds for a quick test run?
> > 
> > The variable is named runtime_ms, but it is not a realtime duration, but
> > simulated runtime. No clocks are tied to it. It turns out, simulation is
~300
> > times faster than realtime. Originally, tests are run with 10 million ms
> there.
> > This value of 100000 is already 100 times less than that.
> 
> Ok
> 
> I would prefer if you moved this to where the Run() method is being called, so
> you can do:
> 
> Run(field_trial::FindFullName("WebRTC-QuickPerfTest") == "Enabled" ?
> kQuickTestTimeoutMs : kRegularTestTimeoutMs, loss_rate, drift_factor);
> 
> That way the caller won't be surprised.

Do you prefer it strongly that way? It will be a lot of changes, because this
Run() method is called from everywhere.

[hlundin-webrtc, 2017-02-21 10:35:04]

I agree with Stefan on the surprise-change of runtime for the NetEq test. But,
why is the NetEq test even modified? The referred bug only talks about rampup
and callperf tests.

https://codereview.webrtc.org/2708723002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/neteq_performance_test.cc (right):

https://codereview.webrtc.org/2708723002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_performance_test.cc:31: const int
kQuickTestTimeoutMs = 100000;
On 2017/02/21 10:26:03, stefan-webrtc wrote:
> On 2017/02/21 09:46:08, ilnik wrote:
> > On 2017/02/21 09:35:46, stefan-webrtc wrote:
> > > Sounds like a lot to have 100 seconds for a quick test run?
> > 
> > The variable is named runtime_ms, but it is not a realtime duration, but
> > simulated runtime. No clocks are tied to it. It turns out, simulation is
~300
> > times faster than realtime. Originally, tests are run with 10 million ms
> there.
> > This value of 100000 is already 100 times less than that.
> 
> Ok
> 
> I would prefer if you moved this to where the Run() method is being called, so
> you can do:
> 
> Run(field_trial::FindFullName("WebRTC-QuickPerfTest") == "Enabled" ?
> kQuickTestTimeoutMs : kRegularTestTimeoutMs, loss_rate, drift_factor);
> 
> That way the caller won't be surprised.

Yes, agree with Stefan. This is an unexpected override of the passed parameter.

[ilnik, 2017-02-21 10:37:41]

On 2017/02/21 10:35:04, hlundin-webrtc wrote:
> I agree with Stefan on the surprise-change of runtime for the NetEq test. But,
> why is the NetEq test even modified? The referred bug only talks about rampup
> and callperf tests.
> 
>
https://codereview.webrtc.org/2708723002/diff/20001/webrtc/modules/audio_codi...
> File webrtc/modules/audio_coding/neteq/tools/neteq_performance_test.cc
(right):
> 
>
https://codereview.webrtc.org/2708723002/diff/20001/webrtc/modules/audio_codi...
> webrtc/modules/audio_coding/neteq/tools/neteq_performance_test.cc:31: const
int
> kQuickTestTimeoutMs = 100000;
> On 2017/02/21 10:26:03, stefan-webrtc wrote:
> > On 2017/02/21 09:46:08, ilnik wrote:
> > > On 2017/02/21 09:35:46, stefan-webrtc wrote:
> > > > Sounds like a lot to have 100 seconds for a quick test run?
> > > 
> > > The variable is named runtime_ms, but it is not a realtime duration, but
> > > simulated runtime. No clocks are tied to it. It turns out, simulation is
> ~300
> > > times faster than realtime. Originally, tests are run with 10 million ms
> > there.
> > > This value of 100000 is already 100 times less than that.
> > 
> > Ok
> > 
> > I would prefer if you moved this to where the Run() method is being called,
so
> > you can do:
> > 
> > Run(field_trial::FindFullName("WebRTC-QuickPerfTest") == "Enabled" ?
> > kQuickTestTimeoutMs : kRegularTestTimeoutMs, loss_rate, drift_factor);
> > 
> > That way the caller won't be surprised.
> 
> Yes, agree with Stefan. This is an unexpected override of the passed
parameter.

Ok, will change that.

The goal is to make all perf tests quick enough. NetEq test was taking 45
seconds - too long to be ran on trybots. 
But, you are right, we have to update referred bug.

[hlundin-webrtc, 2017-02-21 11:27:30]

On 2017/02/21 10:37:41, ilnik wrote:
> On 2017/02/21 10:35:04, hlundin-webrtc wrote:
> > I agree with Stefan on the surprise-change of runtime for the NetEq test.
But,
> > why is the NetEq test even modified? The referred bug only talks about
rampup
> > and callperf tests.
> > 
> >
>
https://codereview.webrtc.org/2708723002/diff/20001/webrtc/modules/audio_codi...
> > File webrtc/modules/audio_coding/neteq/tools/neteq_performance_test.cc
> (right):
> > 
> >
>
https://codereview.webrtc.org/2708723002/diff/20001/webrtc/modules/audio_codi...
> > webrtc/modules/audio_coding/neteq/tools/neteq_performance_test.cc:31: const
> int
> > kQuickTestTimeoutMs = 100000;
> > On 2017/02/21 10:26:03, stefan-webrtc wrote:
> > > On 2017/02/21 09:46:08, ilnik wrote:
> > > > On 2017/02/21 09:35:46, stefan-webrtc wrote:
> > > > > Sounds like a lot to have 100 seconds for a quick test run?
> > > > 
> > > > The variable is named runtime_ms, but it is not a realtime duration, but
> > > > simulated runtime. No clocks are tied to it. It turns out, simulation is
> > ~300
> > > > times faster than realtime. Originally, tests are run with 10 million ms
> > > there.
> > > > This value of 100000 is already 100 times less than that.
> > > 
> > > Ok
> > > 
> > > I would prefer if you moved this to where the Run() method is being
called,
> so
> > > you can do:
> > > 
> > > Run(field_trial::FindFullName("WebRTC-QuickPerfTest") == "Enabled" ?
> > > kQuickTestTimeoutMs : kRegularTestTimeoutMs, loss_rate, drift_factor);
> > > 
> > > That way the caller won't be surprised.
> > 
> > Yes, agree with Stefan. This is an unexpected override of the passed
> parameter.
> 
> Ok, will change that.
> 
> The goal is to make all perf tests quick enough. NetEq test was taking 45
> seconds - too long to be ran on trybots. 
> But, you are right, we have to update referred bug.

OK. Just be aware that some tests may need a longer runtime to produce stable
results with low variance. Reducing the runtime may in some cases introduce more
noise, preventing us from detecting smaller regressions.

[hlundin-webrtc, 2017-02-21 11:28:50]

LGTM for NetEq test.

[ilnik, 2017-02-21 11:35:06]

On 2017/02/21 11:28:50, hlundin-webrtc wrote:
> LGTM for NetEq test.

Stefan, what about the rest? Do you approve of it?

[stefan-webrtc, 2017-02-21 11:52:05]

lgtm

[ilnik, 2017-02-21 11:58:06]

The CQ bit was checked by ilnik@webrtc.org

[commit-bot: I haz the power, 2017-02-21 11:58:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-21 13:20:29]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487678286956820,
"parent_rev": "24899e58ecb10157fea418d80022b938c0f806a9", "commit_rev":
"5328b9eb328e049cb19c5b3ce99fdbf2bbae25e5"}

[commit-bot: I haz the power, 2017-02-21 13:20:32]

Description was changed from

==========
added WebRTC-QuickPerfTest to RampUpTests and CallPerfTests

BUG=webrtc:7153
==========

to

==========
added WebRTC-QuickPerfTest to RampUpTests and CallPerfTests

BUG=webrtc:7153

Review-Url: https://codereview.webrtc.org/2708723002
Cr-Commit-Position: refs/heads/master@{#16743}
Committed:
https://chromium.googlesource.com/external/webrtc/+/5328b9eb328e049cb19c5b3ce...
==========

[commit-bot: I haz the power, 2017-02-21 13:20:33]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/5328b9eb328e049cb19c5b3ce...