Closed Bug 624739 Opened 13 years ago Closed 13 years ago

Sort Idle HTTP Connections by CWND

Categories

(Core :: Networking: HTTP, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mcmanus, Assigned: mcmanus)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

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: nobody → mcmanus
Status: NEW → ASSIGNED
Attached patch sort idle pconn pool by cwnd v1 (obsolete) — Splinter Review
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)
Attachment #502808 - Flags: review?(jduell.mcbugs) → review?(honzab.moz)
Attached patch sort idle pconn pool by cwnd v2 (obsolete) — Splinter Review
update bitrot - candidate for firefox 5
Attachment #502808 - Attachment is obsolete: true
Attachment #524549 - Flags: review?
Attachment #502808 - Flags: review?(honzab.moz)
Blocks: 604796
Attachment #524549 - Flags: review? → review?(jduell.mcbugs)
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+
updates from comment 3.. r=jduell
Attachment #524549 - Attachment is obsolete: true
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+
Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/198b15a65432
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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.
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
This is done; filed bug 650563 for updating the HTTP transaction model documentation.
Depends on: 650871
You need to log in before you can comment on or make changes to this bug.