Closed
Bug 624739
Opened 13 years ago
Closed 13 years ago
Sort Idle HTTP Connections by CWND
Categories
(Core :: Networking: HTTP, enhancement)
Core
Networking: HTTP
Tracking
()
RESOLVED
FIXED
People
(Reporter: mcmanus, Assigned: mcmanus)
References
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 2 obsolete files)
7.40 KB,
patch
|
jduell.mcbugs
:
review+
|
Details | Diff | Splinter Review |
Right now the idle persistent connection pool is a FIFO. What really distinguishes different connections to the same server is the size of the sending congestion window (CWND) on the server. If the window is large enough to support the next response document then it can all be transferred (by definition) in 1 RTT. Connections with smaller windows are going to be limited by the RTT while they grow their windows. All else being equal, which as far as I can tell it is, we want to use the big ones. We cannot directly tell what the server's CWND is of course, but the history of the connection provides a clue - connections which have moved large flights of data (single responses, or aggregate pipelines of responses) will have given the server the best chance for opening that window in the past. we should sort the pool according to that metric. I've done an experiment to show the best case - a link to a 25KB resource off of a page that contains a mixture of small and large content. In both cases the 25KB resource is loaded with an idle persistent connection. In the historic case it reuses a connection that had loaded a small image previously and it takes 3RTT (793ms) to transfer it.. in the case of sorting by cwnd the window is large enough to accommodate the entire resource and it is all complete in 1 RTT (363ms). Cool! A much longer description of this along with pretty graphs is here: http://bitsup.blogspot.com/2011/01/firefox-idle-connection-selection-via.html I don't really see a downside - the current algorithm is more or less a random selection and the worst case of sort-by-cwnd devolves into that. The code is also very simple - a lot simpler than this explanation.
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•13 years ago
|
||
this will conflict with the patch in 604796 - but that will be easy to cleanup. I thought it better to submit without a dependency.
Attachment #502808 -
Flags: review?(jduell.mcbugs)
Updated•13 years ago
|
Attachment #502808 -
Flags: review?(jduell.mcbugs) → review?(honzab.moz)
Assignee | ||
Comment 2•13 years ago
|
||
update bitrot - candidate for firefox 5
Attachment #502808 -
Attachment is obsolete: true
Attachment #524549 -
Flags: review?
Attachment #502808 -
Flags: review?(honzab.moz)
Assignee | ||
Updated•13 years ago
|
Attachment #524549 -
Flags: review? → review?(jduell.mcbugs)
Comment 3•13 years ago
|
||
Comment on attachment 524549 [details] [diff] [review] sort idle pconn pool by cwnd v2 >+ PRInt64 mBytesRead; // data read per activation >+ PRInt64 mMaxBytesRead; // max read in 1 activation Change name of mBytesRead to mCurrentBytesRead to make it clear that its the bytes for the current activation, not "per activation." >+ >+ PRInt32 idx; >+ for (idx = 0; idx < ent->mIdleConns.Length(); idx++) { >+ nsHttpConnection *idleConn = ent->mIdleConns[idx]; >+ if (idleConn->MaxBytesRead() < conn->MaxBytesRead()) >+ break; >+ } >+ Add comment that this is inefficient way to sort list, but that's ok since it's only 6 elements long max. +r with those fixes.
Attachment #524549 -
Flags: review?(jduell.mcbugs) → review+
Assignee | ||
Comment 4•13 years ago
|
||
updates from comment 3.. r=jduell
Attachment #524549 -
Attachment is obsolete: true
Assignee | ||
Comment 5•13 years ago
|
||
pushed to cedar http://hg.mozilla.org/projects/cedar/rev/198b15a65432
Comment 6•13 years ago
|
||
Comment on attachment 524586 [details] [diff] [review] sort idle pconn pool by cwnd v3 carry forward review next time (and obsolete prev verison)
Attachment #524586 -
Flags: review+
Comment 7•13 years ago
|
||
Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/198b15a65432
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Keywords: dev-doc-needed
Comment 8•13 years ago
|
||
What developer documentation changes need to be made? There appear to be no exposed API changes in this patch, and the only documentation we have that even mentions the HTTP transaction model is this: https://developer.mozilla.org/en/HTTP_Transaction_Model But this article is very old and is tagged as being badly out of date. If someone would like to tackle a revamp of that article (or provide me with notes that I can use to bring it up to date), that would be fabulous.
Comment 9•13 years ago
|
||
I have added a note to Firefox 5 for developers about this to cover things until/unless that other article gets updated: https://developer.mozilla.org/en/Firefox_5_for_developers#HTTP
Comment 10•13 years ago
|
||
This is done; filed bug 650563 for updating the HTTP transaction model documentation.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•