Feature Request - Queue up larger buffers to send via NPN_Write

RESOLVED FIXED in Firefox 36

Status

()

Core
Plug-ins
--
enhancement
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: jimsonx, Assigned: bsmedberg)

Tracking

Trunk
mozilla37
x86
Windows 7
Points:
1
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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.
(Assignee)

Updated

3 years ago
Component: Untriaged → Plug-ins
Product: Firefox → Core
(Assignee)

Comment 1

3 years ago
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.
Flags: firefox-backlog+
Whiteboard: [p=1][qa-]
(Assignee)

Updated

3 years ago
Flags: needinfo?(bent.mozilla)
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.
Flags: needinfo?(bent.mozilla)
(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.
(Assignee)

Updated

3 years ago
Depends on: 1117964
Depends on: 1116806
(Assignee)

Comment 4

3 years ago
Created attachment 8544681 [details]
MozReview Request: bz://869208/bsmedberg
Attachment #8544681 - Flags: review?(aklotz)
(Assignee)

Comment 5

3 years ago
/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
(Assignee)

Comment 6

3 years ago
Let's make the buffer at least somewhat bigger while we're at it.
Assignee: nobody → benjamin
Status: UNCONFIRMED → NEW
Ever confirmed: true
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?
https://reviewboard.mozilla.org/r/2055/#review1333

Ship It!
Attachment #8544681 - Flags: review?(aklotz) → review+

Comment 9

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

Updated

3 years ago
Attachment #8544681 - Flags: review?(bzbarsky)
Attachment #8544681 - Flags: review?(aklotz)
Attachment #8544681 - Flags: review+
(Assignee)

Comment 10

3 years ago
/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
(Assignee)

Comment 11

3 years ago
/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
(Assignee)

Comment 12

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf207a98cbb9
Target Milestone: --- → mozilla37
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".
Attachment #8544681 - Flags: review?(bzbarsky) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/cf207a98cbb9
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED

Updated

3 years ago
Iteration: --- → 37.3 - 12 Jan
Points: --- → 1
Flags: qe-verify-
Whiteboard: [p=1][qa-]
Attachment #8544681 - Flags: review?(aklotz) → review+
(Assignee)

Comment 16

2 years ago
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
Attachment #8544681 - Flags: approval-mozilla-beta?
status-firefox36: --- → affected
status-firefox37: --- → fixed
Attachment #8544681 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/cb0fd5d9a263
status-firefox36: affected → fixed
(Assignee)

Comment 18

2 years ago
Comment on attachment 8544681 [details]
MozReview Request: bz://869208/bsmedberg
Attachment #8544681 - Attachment is obsolete: true
Attachment #8618014 - Flags: review+
Attachment #8618015 - Flags: review+
(Assignee)

Comment 19

2 years ago
Created attachment 8618014 [details]
MozReview Request: Bug 1119302 - Implement nsIContentPolicy.shouldProcess for plugin subresource loads, r=bz
(Assignee)

Comment 20

2 years ago
Created attachment 8618015 [details]
MozReview Request: Bug 869208 - Increase the buffer size we're using to deliver network streams to OOPP plugins, r=aklotz
You need to log in before you can comment on or make changes to this bug.