Last Comment Bug 624739 - Sort Idle HTTP Connections by CWND
: Sort Idle HTTP Connections by CWND
: dev-doc-complete
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: Trunk
: All All
: -- enhancement with 2 votes (vote)
: ---
Assigned To: Patrick McManus [:mcmanus]
: Patrick McManus [:mcmanus]
Depends on: 650871
Blocks: 604796
  Show dependency treegraph
Reported: 2011-01-11 08:30 PST by Patrick McManus [:mcmanus]
Modified: 2011-07-08 07:02 PDT (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

sort idle pconn pool by cwnd v1 (7.64 KB, patch)
2011-01-11 08:34 PST, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
sort idle pconn pool by cwnd v2 (7.20 KB, patch)
2011-04-07 17:22 PDT, Patrick McManus [:mcmanus]
jduell.mcbugs: review+
Details | Diff | Splinter Review
sort idle pconn pool by cwnd v3 (7.40 KB, patch)
2011-04-07 23:41 PDT, Patrick McManus [:mcmanus]
jduell.mcbugs: review+
Details | Diff | Splinter Review

Description Patrick McManus [:mcmanus] 2011-01-11 08:30:44 PST
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:

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.
Comment 1 Patrick McManus [:mcmanus] 2011-01-11 08:34:17 PST
Created attachment 502808 [details] [diff] [review]
sort idle pconn pool by cwnd v1

this will conflict with the patch in 604796 - but that will be easy to cleanup. I thought it better to submit without a dependency.
Comment 2 Patrick McManus [:mcmanus] 2011-04-07 17:22:46 PDT
Created attachment 524549 [details] [diff] [review]
sort idle pconn pool by cwnd v2

update bitrot - candidate for firefox 5
Comment 3 Jason Duell [:jduell] (needinfo me) 2011-04-07 20:16:53 PDT
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.
Comment 4 Patrick McManus [:mcmanus] 2011-04-07 23:41:10 PDT
Created attachment 524586 [details] [diff] [review]
sort idle pconn pool by cwnd v3

updates from comment 3.. r=jduell
Comment 5 Patrick McManus [:mcmanus] 2011-04-08 11:44:09 PDT
pushed to cedar
Comment 6 Jason Duell [:jduell] (needinfo me) 2011-04-08 12:32:52 PDT
Comment on attachment 524586 [details] [diff] [review]
sort idle pconn pool by cwnd v3

carry forward review next time (and obsolete prev verison)
Comment 7 Benjamin Stover (:stechz) 2011-04-09 13:03:24 PDT
Pushed to m-c:
Comment 8 Eric Shepherd [:sheppy] 2011-04-12 23:28:22 PDT
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:

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 Eric Shepherd [:sheppy] 2011-04-12 23:35:41 PDT
I have added a note to Firefox 5 for developers about this to cover things until/unless that other article gets updated:
Comment 10 Eric Shepherd [:sheppy] 2011-04-16 16:58:43 PDT
This is done; filed bug 650563 for updating the HTTP transaction model documentation.

Note You need to log in before you can comment on or make changes to this bug.