RTCStatsCollector collecting stats on multiple threads.

RTCStatsCollector collecting stats on multiple threads.

Changes GetStatsReport to a callback-based function. Stats collection
is dispatched to three different stats collecting methods, being
invoked asynchronously on the signaling, worker and network threads.
The three resulting stats reports are merged into one before returned.

The only current stats being collected is on the signaling thread, but
a FakeRTCStatsCollector is able to test the multi-threaded and
stats-merging behaviors. Future CLs simply have to put their stats
collecting code in the appropriate ProducePartialResultsOnFooThread
method.

BUG=chromium:627816
Committed: https://crrev.com/c82f2e1613e394f92c56d14c38b3f9069075039e
Cr-Commit-Position: refs/heads/master@{#14064}

[hbos, 2016-08-31 08:14:53]

Patchset #3 (id:40001) has been deleted

[hbos, 2016-08-31 08:15:00]

Patchset #2 (id:20001) has been deleted

[hbos, 2016-08-31 08:15:08]

Patchset #1 (id:1) has been deleted

[hbos, 2016-08-31 08:45:53]

Description was changed from

==========
RTCStatsCollector collecting stats on multiple threads.

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

to

==========
RTCStatsCollector collecting stats on multiple threads.

Changes GetStatsReport to a callback-based function. Stats collection
is dispatched to three different stats collecting methods, being
invoked asynchronously on the signaling, worker and network threads.
The three resulting stats reports are merged into one before returned.

The only current stats being collected is on the signaling thread, but
a FakeRTCStatsCollector is able to test the multi-threaded and
stats-merging behaviors. Future CLs simply have to put their stats
collecting code in the appropriate ProducePartialResultsOnFooThread_f
method.

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

[hbos, 2016-08-31 08:53:20]

hbos@webrtc.org changed reviewers:
+ glaznev@webrtc.org, hta@webrtc.org

[hbos, 2016-08-31 08:53:21]

Please take a look, hta and glaznev.

[hta-webrtc, 2016-08-31 09:57:37]

I had a few comments.... the principle seems fine with me, the implementation
seems to require some more work.

This assumes that all the fields in each statsReport come from a single process.
If that isn't true, you need to do postprocessing in the "all processes have
delivered" function.

https://codereview.webrtc.org/2270033004/diff/80001/webrtc/stats/rtcstatscoll...
File webrtc/stats/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2270033004/diff/80001/webrtc/stats/rtcstatscoll...
webrtc/stats/rtcstatscollector.cc:64: callbacks = callbacks_;
So you use "callbacks" to pass the list of callbacks to be done out of the lock
section. I find this confusing.

I find this confusing. Can you write a helper function called "ReturnData" or
something that actually does the "for each callback, invoke it" part, and then
call that from both this function and AddPartialResults_s?

callbacks_ is protected by lock_, but has a different lock-required pattern than
cached_report_; you may consider using two locks.

https://codereview.webrtc.org/2270033004/diff/80001/webrtc/stats/rtcstatscoll...
webrtc/stats/rtcstatscollector.cc:80: }
Where's the "else" branch (the report isn't fresh and
num_pending_partial_reports is > 0)?

My reading is that this will fall through without doing anything because
"callbacks" is empty, but this isn't obvious from the code structure.

https://codereview.webrtc.org/2270033004/diff/80001/webrtc/stats/rtcstatscoll...
File webrtc/stats/rtcstatscollector.h (right):

https://codereview.webrtc.org/2270033004/diff/80001/webrtc/stats/rtcstatscoll...
webrtc/stats/rtcstatscollector.h:70: virtual void
ProducePartialResultsOnNetworkThread_n(double timestamp);
Is the _s/_n/_w suffix a WebRTC convention that we have to adhere to?
It seems to be breaking naming rules & repeating the same info twice.

https://codereview.webrtc.org/2270033004/diff/80001/webrtc/stats/rtcstatscoll...
webrtc/stats/rtcstatscollector.h:90: rtc::CriticalSection lock_;
Please add GUARDED_BY statements on the stuff that's guarded by this lock. It's
good documentation, and there are tools that do checking for you.

https://codereview.webrtc.org/2270033004/diff/80001/webrtc/stats/rtcstatscoll...
File webrtc/stats/rtcstatscollector_unittest.cc (right):

https://codereview.webrtc.org/2270033004/diff/80001/webrtc/stats/rtcstatscoll...
webrtc/stats/rtcstatscollector_unittest.cc:99: public RTCStatsCollectorCallback
{
If you're faking the implementation under test, that's not a good sign. Is there
a way to test the implementation you're testing (RTCStatsCollector) without
subclassing it?

If not, your class may fail the "testability" sniff test.

https://codereview.webrtc.org/2270033004/diff/80001/webrtc/stats/rtcstatscoll...
webrtc/stats/rtcstatscollector_unittest.cc:245: void InitCollector(bool
fake_collector) {
Not a good code smell. If you really need to have fake_collector, make two
functions: InitCollector and InitFakeCollector.

or even better: Write two test fixture classes - RTCStatsCollectorTest and
RTCStatsCollectorTestWithFakeCollector. The second one can subclass the first
one to reduce code duplication. New test cases can be added to the appropriate
test set, and all those that don't care about the threading model can just go
into the RTCStatsCollectorTest class.

Simple things should be simple.

https://codereview.webrtc.org/2270033004/diff/80001/webrtc/stats/rtcstatscoll...
webrtc/stats/rtcstatscollector_unittest.cc:295: }
Please add a test that calls GetStatsReport() twice in succession (with no
waits), and verify that both callbacks get called.

For extra coverage, call GetStatsReport(), increment the (fake) time by 100 msec
(so that the collector will assume that the first result is stale), call
GetStatsReport again, and see that both callbacks get called. Whether the two
results should have the same timestamp or not is a good question; whether the
result will be the same across 1000 test runs is an even better one.

[hbos, 2016-09-01 14:16:16]

Please take a look, hta.

https://codereview.webrtc.org/2270033004/diff/80001/webrtc/stats/rtcstatscoll...
File webrtc/stats/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2270033004/diff/80001/webrtc/stats/rtcstatscoll...
webrtc/stats/rtcstatscollector.cc:64: callbacks = callbacks_;
On 2016/08/31 09:57:36, hta-webrtc wrote:
> So you use "callbacks" to pass the list of callbacks to be done out of the
lock
> section. I find this confusing.
> 
> I find this confusing. Can you write a helper function called "ReturnData" or
> something that actually does the "for each callback, invoke it" part, and then
> call that from both this function and AddPartialResults_s?
> 
> callbacks_ is protected by lock_, but has a different lock-required pattern
than
> cached_report_; you may consider using two locks.

Ah, only |num_pending_partial_reports_| and |partial_report_| are touched on
multiple threads and need to be guarded. This simplifies the code a lot.

Wrote helper function DeliverCachedReport().

https://codereview.webrtc.org/2270033004/diff/80001/webrtc/stats/rtcstatscoll...
webrtc/stats/rtcstatscollector.cc:80: }
On 2016/08/31 09:57:36, hta-webrtc wrote:
> Where's the "else" branch (the report isn't fresh and
> num_pending_partial_reports is > 0)?
> 
> My reading is that this will fall through without doing anything because
> "callbacks" is empty, but this isn't obvious from the code structure.

callback_ has been added to callbacks_ and will be informed when the pending
stats gathering completes and does DeliverCachedReport(). Added a comment.

https://codereview.webrtc.org/2270033004/diff/80001/webrtc/stats/rtcstatscoll...
File webrtc/stats/rtcstatscollector.h (right):

https://codereview.webrtc.org/2270033004/diff/80001/webrtc/stats/rtcstatscoll...
webrtc/stats/rtcstatscollector.h:70: virtual void
ProducePartialResultsOnNetworkThread_n(double timestamp);
On 2016/08/31 09:57:36, hta-webrtc wrote:
> Is the _s/_n/_w suffix a WebRTC convention that we have to adhere to?
> It seems to be breaking naming rules & repeating the same info twice.

Probably not, I'll skip it for these three.

https://codereview.webrtc.org/2270033004/diff/80001/webrtc/stats/rtcstatscoll...
webrtc/stats/rtcstatscollector.h:90: rtc::CriticalSection lock_;
On 2016/08/31 09:57:36, hta-webrtc wrote:
> Please add GUARDED_BY statements on the stuff that's guarded by this lock.
It's
> good documentation, and there are tools that do checking for you.

Done.

https://codereview.webrtc.org/2270033004/diff/80001/webrtc/stats/rtcstatscoll...
File webrtc/stats/rtcstatscollector_unittest.cc (right):

https://codereview.webrtc.org/2270033004/diff/80001/webrtc/stats/rtcstatscoll...
webrtc/stats/rtcstatscollector_unittest.cc:99: public RTCStatsCollectorCallback
{
On 2016/08/31 09:57:36, hta-webrtc wrote:
> If you're faking the implementation under test, that's not a good sign. Is
there
> a way to test the implementation you're testing (RTCStatsCollector) without
> subclassing it?
> 
> If not, your class may fail the "testability" sniff test.

I wanted to test the threaded aspects of the class as a separate test
("distribute work on different threads, merge the results") without relying on
real stats collection to do so. Currently there are only real stats gathered on
the signaling thread, nothing on the worker or network threads. If we had stats
gathered on all threads, then the tests for those stats would implicitly test
this functionality.

I could put the "distribute work on different threads, merge the results"
functionality into a separate helper class and test that separately, but that
just seems like work-around to avoid the "Fake" keyword.

https://codereview.webrtc.org/2270033004/diff/80001/webrtc/stats/rtcstatscoll...
webrtc/stats/rtcstatscollector_unittest.cc:245: void InitCollector(bool
fake_collector) {
On 2016/08/31 09:57:36, hta-webrtc wrote:
> Not a good code smell. If you really need to have fake_collector, make two
> functions: InitCollector and InitFakeCollector.
> 
> or even better: Write two test fixture classes - RTCStatsCollectorTest and
> RTCStatsCollectorTestWithFakeCollector. The second one can subclass the first
> one to reduce code duplication. New test cases can be added to the appropriate
> test set, and all those that don't care about the threading model can just go
> into the RTCStatsCollectorTest class.
> 
> Simple things should be simple.

Done.

https://codereview.webrtc.org/2270033004/diff/80001/webrtc/stats/rtcstatscoll...
webrtc/stats/rtcstatscollector_unittest.cc:295: }
On 2016/08/31 09:57:36, hta-webrtc wrote:
> Please add a test that calls GetStatsReport() twice in succession (with no
> waits), and verify that both callbacks get called.
> 
> For extra coverage, call GetStatsReport(), increment the (fake) time by 100
msec
> (so that the collector will assume that the first result is stale), call
> GetStatsReport again, and see that both callbacks get called. Whether the two
> results should have the same timestamp or not is a good question; whether the
> result will be the same across 1000 test runs is an even better one.

Done. The FakeClock unfortunately processes messages when you advance time. This
means both that stats gathering is completed and that the cache is invalidated
as a result of advancing "cache lifetime" time. (This caused me a great deal of
confusion.)

[hbos, 2016-09-02 08:17:08]

I got rid of the lock_ altogether, please take another look hta.

[hbos, 2016-09-02 08:39:58]

Description was changed from

==========
RTCStatsCollector collecting stats on multiple threads.

Changes GetStatsReport to a callback-based function. Stats collection
is dispatched to three different stats collecting methods, being
invoked asynchronously on the signaling, worker and network threads.
The three resulting stats reports are merged into one before returned.

The only current stats being collected is on the signaling thread, but
a FakeRTCStatsCollector is able to test the multi-threaded and
stats-merging behaviors. Future CLs simply have to put their stats
collecting code in the appropriate ProducePartialResultsOnFooThread_f
method.

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

to

==========
RTCStatsCollector collecting stats on multiple threads.

Changes GetStatsReport to a callback-based function. Stats collection
is dispatched to three different stats collecting methods, being
invoked asynchronously on the signaling, worker and network threads.
The three resulting stats reports are merged into one before returned.

The only current stats being collected is on the signaling thread, but
a FakeRTCStatsCollector is able to test the multi-threaded and
stats-merging behaviors. Future CLs simply have to put their stats
collecting code in the appropriate ProducePartialResultsOnFooThread
method.

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

[hta-webrtc, 2016-09-02 08:47:13]

lgtm

That fake clock was interesting - modify the global environment in a scoped
variable.... should work.

https://codereview.webrtc.org/2270033004/diff/200001/webrtc/stats/rtcstatscol...
File webrtc/stats/rtcstatscollector.h (right):

https://codereview.webrtc.org/2270033004/diff/200001/webrtc/stats/rtcstatscol...
webrtc/stats/rtcstatscollector.h:22: #include "webrtc/base/thread_annotations.h"
Now that the locks are gone, can you delete this include?

https://codereview.webrtc.org/2270033004/diff/200001/webrtc/stats/rtcstatscol...
File webrtc/stats/rtcstatscollector_unittest.cc (right):

https://codereview.webrtc.org/2270033004/diff/200001/webrtc/stats/rtcstatscol...
webrtc/stats/rtcstatscollector_unittest.cc:240: }  // namespace
It's good practice to leave the tests in the anonymous namespace, I think.

[hbos, 2016-09-02 09:18:30]

Please take a look, glaznev.

https://codereview.webrtc.org/2270033004/diff/200001/webrtc/stats/rtcstatscol...
File webrtc/stats/rtcstatscollector.h (right):

https://codereview.webrtc.org/2270033004/diff/200001/webrtc/stats/rtcstatscol...
webrtc/stats/rtcstatscollector.h:22: #include "webrtc/base/thread_annotations.h"
On 2016/09/02 08:47:13, hta-webrtc wrote:
> Now that the locks are gone, can you delete this include?

Done.

https://codereview.webrtc.org/2270033004/diff/200001/webrtc/stats/rtcstatscol...
File webrtc/stats/rtcstatscollector_unittest.cc (right):

https://codereview.webrtc.org/2270033004/diff/200001/webrtc/stats/rtcstatscol...
webrtc/stats/rtcstatscollector_unittest.cc:240: }  // namespace
On 2016/09/02 08:47:13, hta-webrtc wrote:
> It's good practice to leave the tests in the anonymous namespace, I think.

Done.

[AlexG, 2016-09-02 17:25:58]

lgtm

[hbos, 2016-09-05 07:54:06]

The CQ bit was checked by hbos@webrtc.org

[hbos, 2016-09-05 07:54:07]

The patchset sent to the CQ was uploaded after l-g-t-m from hta@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2270033004/#ps220001 (title:
"Addressed comments")

[commit-bot: I haz the power, 2016-09-05 07:54:13]

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, 2016-09-05 08:36:52]

Description was changed from

==========
RTCStatsCollector collecting stats on multiple threads.

Changes GetStatsReport to a callback-based function. Stats collection
is dispatched to three different stats collecting methods, being
invoked asynchronously on the signaling, worker and network threads.
The three resulting stats reports are merged into one before returned.

The only current stats being collected is on the signaling thread, but
a FakeRTCStatsCollector is able to test the multi-threaded and
stats-merging behaviors. Future CLs simply have to put their stats
collecting code in the appropriate ProducePartialResultsOnFooThread
method.

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

to

==========
RTCStatsCollector collecting stats on multiple threads.

Changes GetStatsReport to a callback-based function. Stats collection
is dispatched to three different stats collecting methods, being
invoked asynchronously on the signaling, worker and network threads.
The three resulting stats reports are merged into one before returned.

The only current stats being collected is on the signaling thread, but
a FakeRTCStatsCollector is able to test the multi-threaded and
stats-merging behaviors. Future CLs simply have to put their stats
collecting code in the appropriate ProducePartialResultsOnFooThread
method.

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

[commit-bot: I haz the power, 2016-09-05 08:36:53]

Committed patchset #9 (id:220001)

[commit-bot: I haz the power, 2016-09-05 08:36:57]

Description was changed from

==========
RTCStatsCollector collecting stats on multiple threads.

Changes GetStatsReport to a callback-based function. Stats collection
is dispatched to three different stats collecting methods, being
invoked asynchronously on the signaling, worker and network threads.
The three resulting stats reports are merged into one before returned.

The only current stats being collected is on the signaling thread, but
a FakeRTCStatsCollector is able to test the multi-threaded and
stats-merging behaviors. Future CLs simply have to put their stats
collecting code in the appropriate ProducePartialResultsOnFooThread
method.

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

to

==========
RTCStatsCollector collecting stats on multiple threads.

Changes GetStatsReport to a callback-based function. Stats collection
is dispatched to three different stats collecting methods, being
invoked asynchronously on the signaling, worker and network threads.
The three resulting stats reports are merged into one before returned.

The only current stats being collected is on the signaling thread, but
a FakeRTCStatsCollector is able to test the multi-threaded and
stats-merging behaviors. Future CLs simply have to put their stats
collecting code in the appropriate ProducePartialResultsOnFooThread
method.

BUG=chromium:627816

Committed: https://crrev.com/c82f2e1613e394f92c56d14c38b3f9069075039e
Cr-Commit-Position: refs/heads/master@{#14064}
==========

[commit-bot: I haz the power, 2016-09-05 08:36:57]

Patchset 9 (id:??) landed as
https://crrev.com/c82f2e1613e394f92c56d14c38b3f9069075039e
Cr-Commit-Position: refs/heads/master@{#14064}