implement HTTP/1.1 pipelining

gonna try for 0.9.5

so, this patch just lays out some supporting framework for pipelining. it abstracts the http transaction and connection objects in preparation for making a nsHttpTransactionPipeline which implements both abstract base classes and acts as a transaction to the real connection and as a connection to each of the pipelined transactions.

Comment on attachment 51303 [details] [diff] [review] patch: adds abstract base classes for nsHttpTransaction and nsHttpConnection r=gagan

the implementation of pipelining is probably going to be post 0.9.5, but i'd really like to get this initial framework patch in place for 0.9.5.

Comment on attachment 51303 [details] [diff] [review] patch: adds abstract base classes for nsHttpTransaction and nsHttpConnection sr=rpotts@netscape.com

OK... checked in the this patch on the trunk.

-> 0.9.6 (for the actual pipelining implementation)

-> 0.9.9 (if time permits)

most likely a post mozilla 1.0 feature.

nope.. will work on a "disabled by default" solution for moz 1.0

things that need to be done: - make sure we don't include toplevel requests in a pipeline - re-verify all race conditions are covered - experimental build(s) initial performance results are encouraging... 1.5-3% on cowtools. no improvement on ibench. visibly noticeable improvement however on slow/high-latency sites.

o running on a 850Mhz, 256Mb RAM, Win2k machine o max 4 requests per pipeline o 1.5-3% page load improvement over fast network connection it'd be very interesting if there was a way to run cowtools over a slow/high-latency connection.

darin: get jrgm to show you how to add bandwith limiting on cowtools. You can reasonably go to 56.6, IIRC, before the data starts getting choppy. That doesn't affect latency, though.

i should add that each line in this log file (minus the ones that say restarting transaction) correspond to a call to nsHttpHandler:: ProcessTransactionQ, which is called whenever there are pending transactions and a "keep-alive" connection completes a transaction (or a pipeline of transactions). so, 96 out of the 125 times that we tried to process pending transactions, we were able to pipeline or process more than one at a time :-) the restarts on the other hand generally correspond to connections dropped by the server after we write some data to the socket. TCP/IP doesn't give us a mechanism to detect failure until we try to actually read from the network. reading EOF (before reading any data) forces the transactions to "restart." restarts obviously hurt performance, and they are ever more painful when pipelining since we have to restart not 1 but several transactions :( this is one reason to limit the size of the pipeline.

still needs some more tweaking.

Why the nsC* stuff? Can't they all go in one combined header file, such that the header file itsself isn't frozen, but the stuff in there will continue to be the same? You could have a spearate header file for frozen vs non forzen interfaces if you want. Although we're only freezing contract ids, not classids anyway, aren't we?

OOps - ignore that comment, made it on the wrong bug.

fixed a few more things. added code to prevent pipelining toplevel documents (this may need some work, but i think it's good enough for now).

Squid which is not 100% http/1.1 compatible supports http pipelining. This patch checks proxy's http version and doesnt use pipelining with squid :( I hacked small patch that allow user to override http/1.1 check with pref "network.http.proxy.pipelining" netwerk/protocol/http/src/nsHttpHandler.cpp: - if (conn->SupportsPipelining() && --- + if (((conn->ConnectionInfo()->UsingHttpProxy() && + (mProxyCapabilities & NS_HTTP_ALLOW_PIPELINING)) || + conn->SupportsPipelining()) &&

Squid only claims to be HTTP/1.0, though, not HTTP/1.1

Comment on attachment 78772 [details] [diff] [review] v1 final phew! r=gagan looks good! You might want to add comments around the #if 0 for partial content on pipelining. (for now)

Should the new file nsHttpPipeline.h be tri-licensed instead of MPL? (Rebuilding with the patch now...)

Hmm, bad news I'm afraid. Five minutes of browsing revealed that someone somewhere is getting confused. Browsing a site served by Microsoft-IIS/5.0 via proxy (Traffic-Server/4.0.15-M-12744 [cMs f ]) things seem okay for a little while but then I find that objects are turning up in the wrong places, ie. I follow a link to an HTML page and instead I get taken to an image that was inlined in the previous page. (This association between the URL and 'wrong' object then sticks until I clear the cache.) I don't know if it's specific to this server; it was simply the first such problem I came across. I will continue testing.

Hmm, this is actually pretty easy to reproduce (albeit not at will) and not specific to that server (same w/ Apache 1.3.23 and 1.3.3). It is most easily 'tickled' by simply going from one page (with, say, lots of inline images) to another (not necessarily on the same server) before the first page finishes downloading. This will cause either the page you've just left or the page you're just going to to be replaced by another object that was loading at the time. n.b. I'm on Linux/x86 with a fairly slow dial-up connection.

adam: the fact that you are connecting via a proxy server suggests that the problem might in fact be due to a bad pipelining implementation on the proxy server. any chance you could test w/o the proxy server?

Well! My ISP forces me through a transparent 'Traffic-Server/4.0.15-M-12744' proxy for port 80 accesses unless I specify my own proxy; unfortunately the only other proxy I know I can use is a Squid/2.4.STABLE4 proxy that only seems to support HTTP/1.0 so I wouldn't be seeing pipelining anyway, I believe. If someone could point me towards a public proxy I can test through which should definitely be able to do pipelining, then I'd be grateful. Alternatively, simply some heavy sites which are on uncommon port numbers.

you can setup apache w/ mod_proxy, which should support pipelining.

I could, but I'd have to set it up locally serving local content, which wouldn't buy me anything (correct me if I'm wrong). I'd lose the high-latency dynamic too, which may, assuming that this is a real problem, be just what's triggering it.

adam: no problem.. what i should have said was "can you please generate a HTTP log while reproducing the problem?" setenv NSPR_LOG_MODULES nsHttp:5 setenv NSPR_LOG_FILE http.log thx!

In the constructor for nsHttpConnection. Don't you need a NS_INIT_ISUPPORTS();

Comment on attachment 78772 [details] [diff] [review] v1 final sr=rpotts@netscape.com looks great!!

Oops .. Of course, in the above post I meant nsHttpPipeline::nsHttpPipeline(). (and not nsHttpConnection)

The patch no longer applies cleanly to the trunk due to the following hunk for nsHttpConnection.cpp. The fix is pretty simple of course. *************** *** 185,191 **** if (!val) val = responseHead->PeekHeader(nsHttp::Proxy_Connection); - if ((responseHead->Version() < NS_HTTP_VERSION_1_1) || (nsHttpHandler::get()->DefaultVersion() < NS_HTTP_VERSION_1_1)) { // HTTP/1.0 connections are by default NOT persistent if (val && !PL_strcasecmp(val, "keep-alive")) --- 186,194 ---- if (!val) val = responseHead->PeekHeader(nsHttp::Proxy_Connection); + mServerVersion = responseHead->Version(); + + if ((mServerVersion < NS_HTTP_VERSION_1_1) || (nsHttpHandler::get()->DefaultVersion() < NS_HTTP_VERSION_1_1)) { // HTTP/1.0 connections are by default NOT persistent if (val && !PL_strcasecmp(val, "keep-alive")) I'm rebuilding with the patch in again so I can get that log -- not long now I hope.

Aw. The patch doesn't compile versus trunk either. :) nsHttpHandler.cpp: In method `nsresult nsHttpHandler::nsPipelineEnqueueState::Init(nsHttpTransaction *)': nsHttpHandler.cpp:2136: cannot allocate an object of type `nsHttpPipeline' nsHttpHandler.cpp:2136: since the following virtual functions are abstract: nsAHttpConnection.h:24: nsresult nsAHttpConnection::OnHeadersAvailable(nsAHttpTransaction *, nsHttpRequestHead *, nsHttpResponseHead *, PRBool *) I'll rewind my tree a couple of days.

I normally run under WindowMaker 0.65 I have just logged in with twm (Xfree86 4.2.0) I get exactly the same behavior with this very venerable, standard windowmanager. I can try others if you wish. The odd thing is the LOSS of focus. Ie space works once then stops. You need to click ONCE before each space. OR Click+Tab to get the window to work correctly. I'll try the new RC1 to see what happens as well...

Okay, I have a log of the error ocurring. About to attach. What I did: 0) Cleared caches, closed down mozilla 1) Restarted, went to http://www.linuxgames.com 2) Before page finished downloading, clicked on 'screenshots' link under heading 'WineX 2.0, Referral Program, and Naming Contest' 3) This takes me to http://www.transgaming.com/screenshots.php?gameid=29 ; before this page finished loading I hit the browser 'back' button. 4) URL goes back to http://www.linuxgames.com but what I actually get is a single GIF image (with the usual image window title, ie. 'GIF image, 12x6 pixels'), in this case I think it was in reality the image at http://www.linuxgames.com/images/menu_middle_right.gif which is one of the images inlined from http://www.linuxgames.com but obviously not what I should be seeing. I then hit the 'reload' button and in this case the 'real' http://www.linuxgames.com page reloaded. If you would like more variations on this log then I can oblige; it's just a bit tricky to make it happen 'first time' so the log doesn't get massive and unreadable.

gzipped for compactness.

n.b. I might have actually gone forward and backwards twice before the problem triggered. I don't think so, but I spent so many sessions going backwards and forwards to produce a 'clean' log that my muscle-memory might have outstripped my memory-memory. An expert should be able to tell quite simply from reading the log, however. I just wanted to clarify.

Harshal: good catch!

adam: thx for the log file!

ok, so i've landed this patch on the trunk (fixed a few things). again, pipelining is by default disabled. adam: can you see if you're still able to repro the problem you're seeing w/ tomorrows build? i suspect you will be able to... can you file a new bug for that problem? thx!

requires follow up patch in bug 138754.

no changes in this patch... it just includes the regression fix from bug 138754.

I've been seeing some sporadic 500 gateway timeouts since I turned this on. I don't know anything about this, so I can't say if it's related or not ? I haven't spotted any of the problems mentioned here (I don't use a proxy). I tried to do some real tests with this on (I'm on a 56k crappy line modem in the UK), but any actual data was completely lost in the noise :/

Comment on attachment 81194 [details] [diff] [review] patch for mozilla 1.0 branch a=chofmann off by default for the 1.0 branch.

adding adt1.0.0+. Please check into the branch as soon as possible and add the fixed1.0.0 keyword.

fixed-on-branch

verified trunk and branch - http pipelining implemented and is off by default: win NT4, linux rh6, mac osX

Hi! Just a quick note to mention that I do still see the weirdness with pipelining enabled; I'll file that bug and see if I can get some better reproduction steps when I scape up some spare time.

Shouldn't this project get marked as complete on http://komodo.mozilla.org/planning/branches.cgi ?

forgot to remove fixed1.0.0 keyword so doing so now