Deliver webrtc-internals updates in bulk, every 500ms at most.

Deliver webrtc-internals updates in bulk, every 500ms at most.
This approach bulks together update notifications to avoid DOSing ourselves when there's lots of peerconnection activity.

BUG=578839
Committed: https://crrev.com/75d54fe84473d8a17cf4a28d05fa2fc6d104ab54
Cr-Commit-Position: refs/heads/master@{#374373}

[tommi (sloooow) - chröme, 2016-02-06 16:55:12]

tommi@chromium.org changed reviewers:
+ grunell@chromium.org

[Henrik Grunell, 2016-02-08 09:52:08]

Great change.

https://codereview.chromium.org/1676043002/diff/1/content/browser/media/webrt...
File content/browser/media/webrtc_internals.cc (right):

https://codereview.chromium.org/1676043002/diff/1/content/browser/media/webrt...
content/browser/media/webrtc_internals.cc:107: SendUpdate("addPeerConnection",
dict->CreateDeepCopy());
Why do you need to create a deep copy? Could the ownership own |dict| just be
transferred?

https://codereview.chromium.org/1676043002/diff/1/content/browser/media/webrt...
content/browser/media/webrtc_internals.cc:367: if (!pending_updates_queued_) {
Could you just check is queue is empty? (Before pushing of course.)

https://codereview.chromium.org/1676043002/diff/1/content/browser/media/webrt...
File content/browser/media/webrtc_internals.h (right):

https://codereview.chromium.org/1676043002/diff/1/content/browser/media/webrt...
content/browser/media/webrtc_internals.h:204: class PendingUpdate {
Comment on the new class and variables. Threading.

https://codereview.chromium.org/1676043002/diff/1/content/renderer/media/peer...
File content/renderer/media/peer_connection_tracker.cc (right):

https://codereview.chromium.org/1676043002/diff/1/content/renderer/media/peer...
content/renderer/media/peer_connection_tracker.cc:624: std::string update_type;
Nit: Remove "using std::string"?

https://codereview.chromium.org/1676043002/diff/1/content/renderer/media/peer...
content/renderer/media/peer_connection_tracker.cc:685: ++next_lid_;
The name |next_lid_| isn't correct anymore, since it no longer holds the next
id, but the last id used. Maybe rename or use it as next id as before? Btw, I
think "l" should be printed out to make it more readable. "next_local_id_"?

https://codereview.chromium.org/1676043002/diff/1/content/renderer/media/peer...
File content/renderer/media/peer_connection_tracker.h (right):

https://codereview.chromium.org/1676043002/diff/1/content/renderer/media/peer...
content/renderer/media/peer_connection_tracker.h:179: void
SendPeerConnectionUpdate(int local_id,
Nit: Would be great to add a comment for this function while at it. For example,
what is |callback_type|?

[tommi (sloooow) - chröme, 2016-02-08 13:27:15]

Address comments, fix tests, change the way we test the WebRTCInternals class.

[tommi (sloooow) - chröme, 2016-02-08 13:47:51]

Add documentation.

[tommi (sloooow) - chröme, 2016-02-08 13:50:23]

Addressed all comments.  I also changed the way we test the WebRTCInternals
class. It's simpler now and the tests are guaranteed not to interfere with each
other's state since each gets its own copy.  Subsequently removed the
ResetForTesting method since we don't need it.

https://codereview.chromium.org/1676043002/diff/1/content/browser/media/webrt...
File content/browser/media/webrtc_internals.cc (right):

https://codereview.chromium.org/1676043002/diff/1/content/browser/media/webrt...
content/browser/media/webrtc_internals.cc:107: SendUpdate("addPeerConnection",
dict->CreateDeepCopy());
On 2016/02/08 09:52:08, Henrik Grunell wrote:
> Why do you need to create a deep copy? Could the ownership own |dict| just be
> transferred?

ownership of dict lies with peer_connection_data_ (see line 103), so we need a
copy.

https://codereview.chromium.org/1676043002/diff/1/content/browser/media/webrt...
content/browser/media/webrtc_internals.cc:367: if (!pending_updates_queued_) {
On 2016/02/08 09:52:08, Henrik Grunell wrote:
> Could you just check is queue is empty? (Before pushing of course.)

We won't be empty on entry in many cases. E.g. in the example case from the bug,
we might have tens of queued up updates already when the method is called.

https://codereview.chromium.org/1676043002/diff/1/content/renderer/media/peer...
File content/renderer/media/peer_connection_tracker.cc (right):

https://codereview.chromium.org/1676043002/diff/1/content/renderer/media/peer...
content/renderer/media/peer_connection_tracker.cc:624: std::string update_type;
On 2016/02/08 09:52:08, Henrik Grunell wrote:
> Nit: Remove "using std::string"?

Changed this to just be |string| like it was.  I had changed it to be const
char* briefly but went back to string and wrote std:: out of habit.

https://codereview.chromium.org/1676043002/diff/1/content/renderer/media/peer...
content/renderer/media/peer_connection_tracker.cc:685: ++next_lid_;
On 2016/02/08 09:52:08, Henrik Grunell wrote:
> The name |next_lid_| isn't correct anymore, since it no longer holds the next
> id, but the last id used. Maybe rename or use it as next id as before? Btw, I
> think "l" should be printed out to make it more readable. "next_local_id_"?

Done.

https://codereview.chromium.org/1676043002/diff/1/content/renderer/media/peer...
File content/renderer/media/peer_connection_tracker.h (right):

https://codereview.chromium.org/1676043002/diff/1/content/renderer/media/peer...
content/renderer/media/peer_connection_tracker.h:179: void
SendPeerConnectionUpdate(int local_id,
On 2016/02/08 09:52:08, Henrik Grunell wrote:
> Nit: Would be great to add a comment for this function while at it. For
example,
> what is |callback_type|?

Done.

[tommi (sloooow) - chröme, 2016-02-08 14:25:09]

Fix gn build

[Henrik Grunell, 2016-02-09 08:18:11]

https://codereview.chromium.org/1676043002/diff/1/content/browser/media/webrt...
File content/browser/media/webrtc_internals.cc (right):

https://codereview.chromium.org/1676043002/diff/1/content/browser/media/webrt...
content/browser/media/webrtc_internals.cc:107: SendUpdate("addPeerConnection",
dict->CreateDeepCopy());
On 2016/02/08 13:50:23, tommi-chromium wrote:
> On 2016/02/08 09:52:08, Henrik Grunell wrote:
> > Why do you need to create a deep copy? Could the ownership own |dict| just
be
> > transferred?
> 
> ownership of dict lies with peer_connection_data_ (see line 103), so we need a
> copy.

Acknowledged.

https://codereview.chromium.org/1676043002/diff/1/content/browser/media/webrt...
content/browser/media/webrtc_internals.cc:367: if (!pending_updates_queued_) {
On 2016/02/08 13:50:23, tommi-chromium wrote:
> On 2016/02/08 09:52:08, Henrik Grunell wrote:
> > Could you just check is queue is empty? (Before pushing of course.)
> 
> We won't be empty on entry in many cases. E.g. in the example case from the
bug,
> we might have tens of queued up updates already when the method is called.

I don't really understand how this works I guess. Isn't this the only place were
updates are pushed to the queue? If that's the case, we'll always post the task
when the first update is pushed, no?

https://codereview.chromium.org/1676043002/diff/1/content/browser/media/webrt...
File content/browser/media/webrtc_internals.h (right):

https://codereview.chromium.org/1676043002/diff/1/content/browser/media/webrt...
content/browser/media/webrtc_internals.h:204: class PendingUpdate {
On 2016/02/08 09:52:08, Henrik Grunell wrote:
> Comment on the new class and variables. Threading.

Not addressed yet. I think it's good to have, especially around the queuing and
flag required.

https://codereview.chromium.org/1676043002/diff/1/content/renderer/media/peer...
File content/renderer/media/peer_connection_tracker.cc (right):

https://codereview.chromium.org/1676043002/diff/1/content/renderer/media/peer...
content/renderer/media/peer_connection_tracker.cc:624: std::string update_type;
On 2016/02/08 13:50:23, tommi-chromium wrote:
> On 2016/02/08 09:52:08, Henrik Grunell wrote:
> > Nit: Remove "using std::string"?
> 
> Changed this to just be |string| like it was.  I had changed it to be const
> char* briefly but went back to string and wrote std:: out of habit.

Acknowledged.

https://codereview.chromium.org/1676043002/diff/1/content/renderer/media/peer...
File content/renderer/media/peer_connection_tracker.h (right):

https://codereview.chromium.org/1676043002/diff/1/content/renderer/media/peer...
content/renderer/media/peer_connection_tracker.h:179: void
SendPeerConnectionUpdate(int local_id,
On 2016/02/08 13:50:23, tommi-chromium wrote:
> On 2016/02/08 09:52:08, Henrik Grunell wrote:
> > Nit: Would be great to add a comment for this function while at it. For
> example,
> > what is |callback_type|?
> 
> Done.

Great comment. Thanks!

https://codereview.chromium.org/1676043002/diff/60001/content/browser/media/w...
File content/browser/media/webrtc_internals.h (right):

https://codereview.chromium.org/1676043002/diff/60001/content/browser/media/w...
content/browser/media/webrtc_internals.h:106: protected:
Nice to get rid of the testing function!

[tommi (sloooow) - chröme, 2016-02-09 09:32:02]

Add documentation and thread checker

[tommi (sloooow) - chröme, 2016-02-09 09:34:16]

Thanks.  I still need to figure out the issue with the debug gn bot.  Will ping
back when I've fixed that (and add an owner to review the build file change). 
In the meantime addressed the comments I missed the last time.

https://codereview.chromium.org/1676043002/diff/1/content/browser/media/webrt...
File content/browser/media/webrtc_internals.cc (right):

https://codereview.chromium.org/1676043002/diff/1/content/browser/media/webrt...
content/browser/media/webrtc_internals.cc:367: if (!pending_updates_queued_) {
On 2016/02/09 08:18:11, Henrik Grunell wrote:
> On 2016/02/08 13:50:23, tommi-chromium wrote:
> > On 2016/02/08 09:52:08, Henrik Grunell wrote:
> > > Could you just check is queue is empty? (Before pushing of course.)
> > 
> > We won't be empty on entry in many cases. E.g. in the example case from the
> bug,
> > we might have tens of queued up updates already when the method is called.
> 
> I don't really understand how this works I guess. Isn't this the only place
were
> updates are pushed to the queue? If that's the case, we'll always post the
task
> when the first update is pushed, no?

Ah yes of course you're right, sorry I didn't get it the first time.  Since
we're always on the UI thread we don't need the extra bool. Done.

https://codereview.chromium.org/1676043002/diff/1/content/browser/media/webrt...
File content/browser/media/webrtc_internals.h (right):

https://codereview.chromium.org/1676043002/diff/1/content/browser/media/webrt...
content/browser/media/webrtc_internals.h:204: class PendingUpdate {
On 2016/02/09 08:18:11, Henrik Grunell wrote:
> On 2016/02/08 09:52:08, Henrik Grunell wrote:
> > Comment on the new class and variables. Threading.
> 
> Not addressed yet. I think it's good to have, especially around the queuing
and
> flag required. 

Done.  I added documentation for the class that mentions the state that it holds
and with that information I think the variables are otherwise self explanatory. 
Also added a thread checker.

[Henrik Grunell, 2016-02-09 11:47:48]

This should be the final comments, on comments only.

Great change to the unit test!

https://codereview.chromium.org/1676043002/diff/1/content/browser/media/webrt...
File content/browser/media/webrtc_internals.cc (right):

https://codereview.chromium.org/1676043002/diff/1/content/browser/media/webrt...
content/browser/media/webrtc_internals.cc:367: if (!pending_updates_queued_) {
On 2016/02/09 09:34:16, tommi-chromium wrote:
> On 2016/02/09 08:18:11, Henrik Grunell wrote:
> > On 2016/02/08 13:50:23, tommi-chromium wrote:
> > > On 2016/02/08 09:52:08, Henrik Grunell wrote:
> > > > Could you just check is queue is empty? (Before pushing of course.)
> > > 
> > > We won't be empty on entry in many cases. E.g. in the example case from
the
> > bug,
> > > we might have tens of queued up updates already when the method is called.
> > 
> > I don't really understand how this works I guess. Isn't this the only place
> were
> > updates are pushed to the queue? If that's the case, we'll always post the
> task
> > when the first update is pushed, no?
> 
> Ah yes of course you're right, sorry I didn't get it the first time.  Since
> we're always on the UI thread we don't need the extra bool. Done.

Acknowledged.

https://codereview.chromium.org/1676043002/diff/80001/content/browser/media/w...
File content/browser/media/webrtc_internals.h (right):

https://codereview.chromium.org/1676043002/diff/80001/content/browser/media/w...
content/browser/media/webrtc_internals.h:110: // instance.
Maybe comment that 500 ms is the default used with ctor with no args?

https://codereview.chromium.org/1676043002/diff/80001/content/browser/media/w...
content/browser/media/webrtc_internals.h:213: // us to use an stl container
while not having to wrap it in a scoped_ptr or
Hard to read this sentence. Repeated "while". (Pun not intended.)

https://codereview.chromium.org/1676043002/diff/80001/content/browser/media/w...
content/browser/media/webrtc_internals.h:215: // The class is single threaded,
so must be
Incomplete... :)

[tommi (sloooow) - chröme, 2016-02-09 11:53:40]

Fix the _right_ gn file.

[tommi (sloooow) - chröme, 2016-02-09 11:59:52]

Address comments

[tommi (sloooow) - chröme, 2016-02-09 12:13:53]

OK, I think we're good to go with the GN fix now. Addressed the comments.

https://codereview.chromium.org/1676043002/diff/80001/content/browser/media/w...
File content/browser/media/webrtc_internals.h (right):

https://codereview.chromium.org/1676043002/diff/80001/content/browser/media/w...
content/browser/media/webrtc_internals.h:110: // instance.
On 2016/02/09 11:47:48, Henrik Grunell wrote:
> Maybe comment that 500 ms is the default used with ctor with no args?

Done.

https://codereview.chromium.org/1676043002/diff/80001/content/browser/media/w...
content/browser/media/webrtc_internals.h:213: // us to use an stl container
while not having to wrap it in a scoped_ptr or
On 2016/02/09 11:47:47, Henrik Grunell wrote:
> Hard to read this sentence. Repeated "while". (Pun not intended.)

Done.

https://codereview.chromium.org/1676043002/diff/80001/content/browser/media/w...
content/browser/media/webrtc_internals.h:215: // The class is single threaded,
so must be
On 2016/02/09 11:47:48, Henrik Grunell wrote:
> Incomplete... :)

Done.  I was working over an extremely slow connection and kept context
switching :) It shows.

[Henrik Grunell, 2016-02-09 13:54:24]

lgtm

[tommi (sloooow) - chröme, 2016-02-09 14:50:22]

The CQ bit was checked by tommi@chromium.org

[commit-bot: I haz the power, 2016-02-09 14:50:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1676043002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1676043002/120001

[commit-bot: I haz the power, 2016-02-09 14:56:39]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-02-09 14:57:33]

Description was changed from

==========
Deliver webrtc-internals updates in bulk, every 500ms at most.
This approach bulks together update notifications to avoid DOSing ourselves when
there's lots of peerconnection activity.

BUG=578839
==========

to

==========
Deliver webrtc-internals updates in bulk, every 500ms at most.
This approach bulks together update notifications to avoid DOSing ourselves when
there's lots of peerconnection activity.

BUG=578839

Committed: https://crrev.com/75d54fe84473d8a17cf4a28d05fa2fc6d104ab54
Cr-Commit-Position: refs/heads/master@{#374373}
==========

[commit-bot: I haz the power, 2016-02-09 14:57:34]

Patchset 7 (id:??) landed as
https://crrev.com/75d54fe84473d8a17cf4a28d05fa2fc6d104ab54
Cr-Commit-Position: refs/heads/master@{#374373}