Pepper: Allow plugins to call PPB_UDP_Socket::SendTo multiple times.

Pepper: Allow plugins to call PPB_UDP_Socket::SendTo multiple times.

This changes the implementation to allow multiple SendTo calls to be in
flight at the same time. The plugin can now call SendTo multiple times
before getting PP_ERROR_INPROGRESS. The number of calls is an
implementation detail. Plugins that want maximum performance should
call SendTo continuously until they get an in progress result, then wait
for one or more previous calls to complete before calling Send again.

The resource is changed to queue SendTo callbacks. It checks that the
plugin stays within the limit, which is currently 8.

The browser message filter is changed to call SendTo until the socket is
busy, then queue the sends. It checks that the plugin can't grow the
queue past the limit. In theory, a plugin could go over the limit by
bypassing the resource checks but not saturating the socket. This is
not really a problem.

Adds a test to send multiple messages in the "optional callbacks" case.

BUG=154338
Committed: https://crrev.com/c76a0f987cf778d0cb14da86f132af9a0b200f72
Cr-Commit-Position: refs/heads/master@{#311563}

[bbudge, 2014-10-08 00:27:04]

bbudge@chromium.org changed reviewers:
+ dmichael@chromium.org, yzshen@chromium.org

[dmichael (off chromium), 2014-10-08 17:20:46]

This looks great so far, thank you Bill!

Broader issues:
- Would it make sense to benchmark this to make sure we're seeing improvement,
and maybe help choose the number of slots?
- It seems possible (perhaps unlikely) that this could break existing apps. It
might be worth considering reving the interface (although that would make the
code messier). At a minimum, it would be wise to verify some apps that use UDP
still work. Ones I'm aware of are Airmech and Chromoting, but there are probably
others.

WDYT?

https://codereview.chromium.org/632113003/diff/60001/chrome/test/ppapi/ppapi_...
File chrome/test/ppapi/ppapi_browsertest.cc (right):

https://codereview.chromium.org/632113003/diff/60001/chrome/test/ppapi/ppapi_...
chrome/test/ppapi/ppapi_browsertest.cc:318: LIST_TEST(UDPSocket_ParallelSend))
clang format?

That's unfortunate, but I guess it's mostly harmless.

https://codereview.chromium.org/632113003/diff/60001/content/browser/renderer...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
(right):

https://codereview.chromium.org/632113003/diff/60001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:367:
CallSendTo(address, port, buffer, context);
It looks currently like CallSendTo might call OnSendToCompleted immediately,
while CallSendTo is still on the stack. And...  OnSendToCompleted will again
call CallSendTo, and so on until net returns ERR_IO_PENDING or we run out of
buffers in the queue.

Maybe it would be cleaner to just always push on the stack here, and always call
"CallSendTo".

Then, CallSendTo checks socket_busy_. If it's not busy, it loops through the
queue and sends everything it can.

WDYT?

https://codereview.chromium.org/632113003/diff/60001/ppapi/tests/test_udp_soc...
File ppapi/tests/test_udp_socket.cc (right):

https://codereview.chromium.org/632113003/diff/60001/ppapi/tests/test_udp_soc...
ppapi/tests/test_udp_socket.cc:323: if (callback_type() == PP_OPTIONAL) {
Maybe it would be cleaner to do:
if (callback_type() != PP_OPTIONAL)
  PASS();

https://codereview.chromium.org/632113003/diff/60001/ppapi/tests/test_udp_soc...
ppapi/tests/test_udp_socket.cc:360: sendto_callbacks[i]->GetCallback());
At this point, I don't think sendto_results[i] should ever be
PP_ERROR_INPROGRESS, right?

Maybe it would make sense to tighten the test slightly by making the while an
if, and asserting that sendto_results != PP_ERROR_INPROGRESS?

[yzshen1, 2014-10-08 17:50:24]

It may be a little inconvenient that the plugin has to wait till it sees an
INPROGRESS to know about the sending rate it should use.
(But I don't know whether that matters.)

What do you think?

[yzshen1, 2014-10-08 18:05:38]

https://codereview.chromium.org/632113003/diff/60001/content/browser/renderer...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
(right):

https://codereview.chromium.org/632113003/diff/60001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:440:
scoped_refptr<net::IOBufferWithSize> buffer,
Why do we need this?
Is it because you are concerned that the underlying net::UDPServerSocket doesn't
retain a ref to |buffer| during Send?

https://codereview.chromium.org/632113003/diff/60001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:456:
pending_sends_.pop();
I think we should pop() before calling CallSendTo().

https://codereview.chromium.org/632113003/diff/60001/content/browser/renderer...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h
(right):

https://codereview.chromium.org/632113003/diff/60001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h:97: void
CallSendTo(const net::IPAddressNumber& address,
nit: the two names (DoSendTo and CallSendTo) are too similar, IMO.

https://codereview.chromium.org/632113003/diff/60001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h:138:
bool socket_busy_;
It is not obvious what it means. (reading or writing, etc.)

[bbudge, 2014-10-08 21:44:35]

David: I'll definitely run benchmarks and check some apps that use it before
trying to land this. I'm hoping to avoid versioning the API. I think the only
way it could break an app is if SendTo currently fails, and this would make it
succeed. It seems unlikely. Am I missing something?

Yuzhu: Would it make sense to add an option that allows the plugin to specify
how many slots are available? It might be nice since we are in effect putting a
FIFO in front of the UDP socket, which might add latency. Of course, this would
require a new version of the API.

https://codereview.chromium.org/632113003/diff/60001/chrome/test/ppapi/ppapi_...
File chrome/test/ppapi/ppapi_browsertest.cc (right):

https://codereview.chromium.org/632113003/diff/60001/chrome/test/ppapi/ppapi_...
chrome/test/ppapi/ppapi_browsertest.cc:318: LIST_TEST(UDPSocket_ParallelSend))
On 2014/10/08 17:20:46, dmichael wrote:
> clang format?
> 
> That's unfortunate, but I guess it's mostly harmless.

I'm fix this manually.

https://codereview.chromium.org/632113003/diff/60001/content/browser/renderer...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
(right):

https://codereview.chromium.org/632113003/diff/60001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:367:
CallSendTo(address, port, buffer, context);
On 2014/10/08 17:20:46, dmichael wrote:
> It looks currently like CallSendTo might call OnSendToCompleted immediately,
> while CallSendTo is still on the stack. And...  OnSendToCompleted will again
> call CallSendTo, and so on until net returns ERR_IO_PENDING or we run out of
> buffers in the queue.
> 
> Maybe it would be cleaner to just always push on the stack here, and always
call
> "CallSendTo".
> 
> Then, CallSendTo checks socket_busy_. If it's not busy, it loops through the
> queue and sends everything it can.
> 
> WDYT?

I like it. Done.

https://codereview.chromium.org/632113003/diff/60001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:440:
scoped_refptr<net::IOBufferWithSize> buffer,
On 2014/10/08 18:05:38, yzshen1 wrote:
> Why do we need this?
> Is it because you are concerned that the underlying net::UDPServerSocket
doesn't
> retain a ref to |buffer| during Send?

Yes, it's to keep it alive until the send completes. net::UDPServerSocket
requires the caller to hold the buffer until the callback is called if the
socket can't complete the SendTo immediately.

https://codereview.chromium.org/632113003/diff/60001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:456:
pending_sends_.pop();
On 2014/10/08 18:05:38, yzshen1 wrote:
> I think we should pop() before calling CallSendTo().

I think you are right, especially in light of David's comment in CallSendTo.
Done.

https://codereview.chromium.org/632113003/diff/60001/content/browser/renderer...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h
(right):

https://codereview.chromium.org/632113003/diff/60001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h:97: void
CallSendTo(const net::IPAddressNumber& address,
On 2014/10/08 18:05:38, yzshen1 wrote:
> nit: the two names (DoSendTo and CallSendTo) are too similar, IMO.

Changed to DoPendingSend.

https://codereview.chromium.org/632113003/diff/60001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h:138:
bool socket_busy_;
On 2014/10/08 18:05:38, yzshen1 wrote:
> It is not obvious what it means. (reading or writing, etc.)

Renamed send_in_progress_

https://codereview.chromium.org/632113003/diff/60001/ppapi/tests/test_udp_soc...
File ppapi/tests/test_udp_socket.cc (right):

https://codereview.chromium.org/632113003/diff/60001/ppapi/tests/test_udp_soc...
ppapi/tests/test_udp_socket.cc:323: if (callback_type() == PP_OPTIONAL) {
On 2014/10/08 17:20:46, dmichael wrote:
> Maybe it would be cleaner to do:
> if (callback_type() != PP_OPTIONAL)
>   PASS();

Done.

https://codereview.chromium.org/632113003/diff/60001/ppapi/tests/test_udp_soc...
ppapi/tests/test_udp_socket.cc:360: sendto_callbacks[i]->GetCallback());
On 2014/10/08 17:20:46, dmichael wrote:
> At this point, I don't think sendto_results[i] should ever be
> PP_ERROR_INPROGRESS, right?
> 
> Maybe it would make sense to tighten the test slightly by making the while an
> if, and asserting that sendto_results != PP_ERROR_INPROGRESS?

OK, I think it's probably good to constrain this behavior since we're making
plugin code to rate limit more complex. We will explicitly guarantee that a slot
opens up when another SendTo completes. Perhaps I should update the IDL too.

[dmichael (off chromium), 2014-10-08 22:42:40]

Most of the comments here are just outlining a very slightly different way to
structure it. Let me know what you think.

https://codereview.chromium.org/632113003/diff/60001/content/browser/renderer...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h
(right):

https://codereview.chromium.org/632113003/diff/60001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h:97: void
CallSendTo(const net::IPAddressNumber& address,
On 2014/10/08 21:44:35, bbudge wrote:
> On 2014/10/08 18:05:38, yzshen1 wrote:
> > nit: the two names (DoSendTo and CallSendTo) are too similar, IMO.
> 
> Changed to DoPendingSend.
suggestion: FlushPendingSends?

https://codereview.chromium.org/632113003/diff/60001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.h:138:
bool socket_busy_;
On 2014/10/08 21:44:35, bbudge wrote:
> On 2014/10/08 18:05:38, yzshen1 wrote:
> > It is not obvious what it means. (reading or writing, etc.)
> 
> Renamed send_in_progress_
Maybe you can eliminate this and rely on !pending_sends_.empty()?

https://codereview.chromium.org/632113003/diff/80001/content/browser/renderer...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
(right):

https://codereview.chromium.org/632113003/diff/80001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:375:
DoPendingSend();
I think you can get rid of the if and just always call
DoPendingSend/FlushPendingSends
...and have it check internally if there's anything left in the queue

https://codereview.chromium.org/632113003/diff/80001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:380:
pending_sends_.pop();
...but to do my suggestion, you'd want to not pop pending_sends_ until
OnSendToCompleted

https://codereview.chromium.org/632113003/diff/80001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:436:
scoped_refptr<net::IOBufferWithSize> buffer,
I think you won't need this anymore because the head of the queue is keeping the
buffer valid...

https://codereview.chromium.org/632113003/diff/80001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:450:
DoPendingSend();
And you'll want to pop() right before calling this to put the next thing on the
front

[bbudge, 2014-10-09 00:18:26]

https://codereview.chromium.org/632113003/diff/80001/content/browser/renderer...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
(right):

https://codereview.chromium.org/632113003/diff/80001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:375:
DoPendingSend();
On 2014/10/08 22:42:40, dmichael wrote:
> I think you can get rid of the if and just always call
> DoPendingSend/FlushPendingSends
> ...and have it check internally if there's anything left in the queue

Done.

https://codereview.chromium.org/632113003/diff/80001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:380:
pending_sends_.pop();
On 2014/10/08 22:42:40, dmichael wrote:
> ...but to do my suggestion, you'd want to not pop pending_sends_ until
> OnSendToCompleted

Done.

https://codereview.chromium.org/632113003/diff/80001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:436:
scoped_refptr<net::IOBufferWithSize> buffer,
On 2014/10/08 22:42:40, dmichael wrote:
> I think you won't need this anymore because the head of the queue is keeping
the
> buffer valid...

Done.

https://codereview.chromium.org/632113003/diff/80001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:450:
DoPendingSend();
On 2014/10/08 22:42:40, dmichael wrote:
> And you'll want to pop() right before calling this to put the next thing on
the
> front

Done. Very nice simplification. Thanks!

[dmichael (off chromium), 2014-10-09 15:44:24]

https://codereview.chromium.org/632113003/diff/100001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
(right):

https://codereview.chromium.org/632113003/diff/100001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:388:
OnSendToCompleted(net_result);
Oh, I realized this still has the slight problem of mutual recursion, that I
think is just a bit harder to reason about. (I highly doubt we'd actually blow
up the stack in this case). This is optional, but I would like it better to just
have an explicit loop until you get a ERR_IO_PENDING.

I think you'd want a separate function, something like SendSendToResult, which
would do what OnSendToCompleted does now, except it wouldn't call
FlushPendingSends.

So the loop would keep calling SendTo while it doesn't get PENDING, and
immediately calling SendSendToResult for each SendTo call that completes
synchronously.

OnSendToCompleted would look much simpler. Basically:
{
  SendSendToResult(net_result);
  FlushPendingSends();
}

Make sense? WDYT? Maybe this is personal preference, but it would the logic
easier to follow to my eye.

[yzshen1, 2014-10-09 16:35:26]

"""Yuzhu: Would it make sense to add an option that allows the plugin to specify
how many slots are available? It might be nice since we are in effect putting a
FIFO in front of the UDP socket, which might add latency. Of course, this would
require a new version of the API."""

If we give the plugin such an option, it will need to be a "negotiation" in
which the service side makes the final decision, because we don't want the
plugin to set a huge number.

But I don't think this is a big/blocking issue, just something that we could
think about.

LGTM the current patch. Thanks!

https://codereview.chromium.org/632113003/diff/100001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
(right):

https://codereview.chromium.org/632113003/diff/100001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:372:
FlushPendingSends();
If a send is already pending, we cannot flush here.

[bbudge, 2014-10-09 21:46:17]

https://codereview.chromium.org/632113003/diff/100001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc
(right):

https://codereview.chromium.org/632113003/diff/100001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:372:
FlushPendingSends();
On 2014/10/09 16:35:26, yzshen1 wrote:
> If a send is already pending, we cannot flush here.

Good catch. Added a check to make sure the queue was empty before we can call
FlushPendingSends here.

https://codereview.chromium.org/632113003/diff/100001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_udp_socket_message_filter.cc:388:
OnSendToCompleted(net_result);
On 2014/10/09 15:44:23, dmichael wrote:
> Oh, I realized this still has the slight problem of mutual recursion, that I
> think is just a bit harder to reason about. (I highly doubt we'd actually blow
> up the stack in this case). This is optional, but I would like it better to
just
> have an explicit loop until you get a ERR_IO_PENDING.
> 
> I think you'd want a separate function, something like SendSendToResult, which
> would do what OnSendToCompleted does now, except it wouldn't call
> FlushPendingSends.
> 
> So the loop would keep calling SendTo while it doesn't get PENDING, and
> immediately calling SendSendToResult for each SendTo call that completes
> synchronously.
> 
> OnSendToCompleted would look much simpler. Basically:
> {
>   SendSendToResult(net_result);
>   FlushPendingSends();
> }
> 
> Make sense? WDYT? Maybe this is personal preference, but it would the logic
> easier to follow to my eye.

I like the idea of an explicit loop. I think it can be done without adding
another function. OnSendToCompleted just doesn't call FlushPendingSends any
more, but it does pop the front PendingSend, so as to keep the buffer alive long
enough.

[dmichael (off chromium), 2014-10-10 16:40:36]

lgtm, thanks for being patient with my bikeshedding

[bbudge, 2014-10-10 16:52:44]

On 2014/10/10 16:40:36, dmichael wrote:
> lgtm, thanks for being patient with my bikeshedding

It's not bikeshedding if it started out as a doghouse.

[dmichael (off chromium), 2015-01-14 17:58:36]

Are you still planning to land this? Are you waiting for performance data?

[bbudge, 2015-01-14 22:31:24]

The CQ bit was checked by bbudge@chromium.org

[commit-bot: I haz the power, 2015-01-14 22:32:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/632113003/400001

[bbudge, 2015-01-14 22:32:01]

On 2015/01/14 17:58:36, dmichael wrote:
> Are you still planning to land this? Are you waiting for performance data?

As we discussed, rebasing and landing.

[commit-bot: I haz the power, 2015-01-14 22:42:28]

Committed patchset #10 (id:400001)

[commit-bot: I haz the power, 2015-01-14 22:43:18]

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