869208 - Feature Request - Queue up larger buffers to send via NPN_Write

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, enhancement)

Description

[jimsonx]

Since Flash Player Protected Mode, NPN_WriteReady and NPN_Write calls have been communicated from the plugin-container process to the sandbox process via IPC calls. The IPC overhead is not too much. However, because there are so many NPN_Writes invoked for a file download, these produce results that _are_ a problem.

We think the proper solution lies in increasing the data buffer size sent over via NPN_Write. Right now the maximum buffer size that Firefox sends to the Flash Player is constrained to 16KB. We would like to see this maximum increased. We leave the specific maximum chunk size to you. But even doubling the maximum chunk size should dramatically reduce the number of NPN_Write and NPN_WriteReady calls. Thus improving the download experience for the end user.

Comment 1

[Benjamin Smedberg]

The only question was making sure the IPC pipes didn't fill up in a single message which would cause blocking. bent do you know the size of our pipes or whether we can safely bump this? 64k would be great.

I think we can safely bump http://hg.mozilla.org/mozilla-central/annotate/48eee276b1ee/dom/plugins/ipc/BrowserStreamParent.cpp#l15 to 64k.

If we do have pipe-filling issues, this becomes more complicated and should probably use shmem buffers.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

It's unclear exactly what our buffer sizes are...

Posix systems have some values baked into the kernel and then they're overridable system-wide with some sysctl.conf changes. You can also use setsockopt() with SO_RCVBUF/SO_SNDBUF to alter individual sockets. I don't see anywhere where we do this currently so we must just get the defaults. My Ubuntu64 VM is confusing me though... It's default socket buffer size is set to 229376 but the max size is 131071... So I guess 128k?

On Windows we're apparently setting our size to 4k, though the docs note that that limit is advisory only and there's some kind of automatic system for increasing the socket size dynamically as needed.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #2)
> On Windows... there's some kind of automatic system for increasing the
> socket size dynamically as needed.

That should be 'pipe' size of course. We don't use sockets on Windows.

Comment 4

[Benjamin Smedberg]

Attachment: MozReview Request: bz://869208/bsmedberg

Comment 5

[Benjamin Smedberg]

/r/2055 - Bug 869208 - Increase the buffer size we're using to deliver network streams to OOPP plugins, r=aklotz

Pull down this commit:

hg pull review -r 70d529e493d445c6c4d24f5a647c622a9f5d0a60

Comment 6

[Benjamin Smedberg]

Let's make the buffer at least somewhat bigger while we're at it.

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

https://reviewboard.mozilla.org/r/2055/#review1331

::: dom/plugins/ipc/BrowserStreamParent.cpp
(Diff revision 1)
> -static const int32_t kSendDataChunk = 0x4000;
> +static const int32_t kSendDataChunk = 0xffff;

Simple enough; Do you plan to land this immediately or when async init rides the trains?

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

https://reviewboard.mozilla.org/r/2055/#review1333

Ship It!

Comment 9

[Jeromie Clark]

This is really exciting news.  We're hopeful that this will reduce the volume of that generic Flash hang.  Thanks for making this happen.

Comment 10

[Benjamin Smedberg]

/r/2055 - Bug 869208 - Increase the buffer size we're using to deliver network streams to OOPP plugins, r=aklotz
/r/2213 - Bug 1119291 - Implement nsIContentPolicy.shouldProcess for plugin subresource loads, r=bz

Pull down these commits:

hg pull review -r a73a3322d58336a9f34f2da762ce6b16f3035e0b

Comment 11

[Benjamin Smedberg]

/r/2055 - Bug 869208 - Increase the buffer size we're using to deliver network streams to OOPP plugins, r=aklotz
/r/2213 - Bug 1119302 - Implement nsIContentPolicy.shouldProcess for plugin subresource loads, r=bz

Pull down these commits:

hg pull review -r 9cc25f7ac50a4167d2e18182b3426e5504023a00

Comment 12

[Benjamin Smedberg]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf207a98cbb9

Comment 13

[Boris Zbarsky [:bzbarsky]]

https://reviewboard.mozilla.org/r/2213/#review1385

::: dom/plugins/base/nsPluginStreamListenerPeer.cpp
(Diff revision 2)
> +  nsAutoCString aContentType;

This is preexisting, but this should probably be called "contentType".

Comment 14

[Boris Zbarsky [:bzbarsky]]

https://reviewboard.mozilla.org/r/2053/#review1387

::: dom/plugins/ipc/BrowserStreamParent.cpp
(Diff revision 1)
> -static const int32_t kSendDataChunk = 0x4000;
> +static const int32_t kSendDataChunk = 0xffff;

Why not 0x10000?  In either case, seems ok, but I don't know this code that well.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cf207a98cbb9

Comment 16

[Benjamin Smedberg]

Comment on attachment 8544681 [details]
MozReview Request: bz://869208/bsmedberg

Approval Request Comment
[Feature/regressing bug #]: Original buffer size was in OOPP
[User impact if declined]: This can improve the Flash hang rate
[Describe test coverage new/current, TreeHerder]: Tests pass, and this has baked for a while on nightly and aurora
[Risks and why]: Fairly low risk. This just changes a buffer size.
[String/UUID change made/needed]: None

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cb0fd5d9a263

Comment 19

[Benjamin Smedberg]

Attachment: MozReview Request: Bug 1119302 - Implement nsIContentPolicy.shouldProcess for plugin subresource loads, r=bz

Comment 20

[Benjamin Smedberg]

Attachment: MozReview Request: Bug 869208 - Increase the buffer size we're using to deliver network streams to OOPP plugins, r=aklotz