Closed Bug 869208 Opened 12 years ago Closed 10 years ago

Feature Request - Queue up larger buffers to send via NPN_Write

Categories

(Core Graveyard :: Plug-ins, enhancement)

x86
Windows 7
enhancement
Not set
normal

Tracking

(firefox36 fixed, firefox37 fixed)

RESOLVED FIXED
mozilla37
Tracking Status
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: jimsonx, Assigned: benjamin)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Component: Untriaged → Plug-ins
Product: Firefox → Core
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-]
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.
Depends on: 1117964
Attached file MozReview Request: bz://869208/bsmedberg (obsolete) (deleted) —
Attachment #8544681 - Flags: review?(aklotz)
/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
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?
Attachment #8544681 - Flags: review?(aklotz) → review+
This is really exciting news. We're hopeful that this will reduce the volume of that generic Flash hang. Thanks for making this happen.
Attachment #8544681 - Flags: review?(bzbarsky)
Attachment #8544681 - Flags: review?(aklotz)
Attachment #8544681 - Flags: review+
/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
/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
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.
Iteration: --- → 37.3 - 12 Jan
Points: --- → 1
Flags: qe-verify-
Whiteboard: [p=1][qa-]
Attachment #8544681 - Flags: review?(aklotz) → review+
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?
Attachment #8544681 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8544681 - Attachment is obsolete: true
Attachment #8618014 - Flags: review+
Attachment #8618015 - Flags: review+
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: