Last Comment Bug 93054 - implement HTTP/1.1 pipelining
: implement HTTP/1.1 pipelining
Status: VERIFIED FIXED
[adt2] [fixed-trunk]
: perf, topembed
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: Trunk
: All All
: P3 enhancement with 1 vote (vote)
: mozilla1.0
Assigned To: Darin Fisher
: Tom Everingham
: Patrick McManus [:mcmanus]
Mentors:
Depends on: 82873 99562
Blocks: 95314 138000 grouper
  Show dependency treegraph
 
Reported: 2001-08-01 00:44 PDT by Darin Fisher
Modified: 2014-04-26 03:33 PDT (History)
33 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch: adds abstract base classes for nsHttpTransaction and nsHttpConnection (33.50 KB, patch)
2001-09-28 15:15 PDT, Darin Fisher
gagan: review+
rpotts: superreview+
Details | Diff | Splinter Review
preliminary patch - works, but still needs a little tweaking (47.72 KB, patch)
2002-04-10 00:34 PDT, Darin Fisher
no flags Details | Diff | Splinter Review
HTTP log output (filtered) running 1 cycle of cowtools w/ pipelining enabled (6.16 KB, text/plain)
2002-04-10 15:41 PDT, Darin Fisher
no flags Details
revised patch - fixed some stability and reliability bugs (48.88 KB, patch)
2002-04-10 17:45 PDT, Darin Fisher
no flags Details | Diff | Splinter Review
v1 final (53.30 KB, patch)
2002-04-11 13:41 PDT, Darin Fisher
gagan: review+
rpotts: superreview+
Details | Diff | Splinter Review
moz HTTP log while weirdness occurs with pipelining on. (79.72 KB, application/octet-stream)
2002-04-19 07:05 PDT, Adam D. Moss
no flags Details
patch for mozilla 1.0 branch (66.61 KB, patch)
2002-04-26 12:49 PDT, Darin Fisher
chofmann: approval+
Details | Diff | Splinter Review

Description Darin Fisher 2001-08-01 00:44:02 PDT
implement HTTP/1.1 pipelining
Comment 1 Darin Fisher 2001-09-06 18:43:00 PDT
gonna try for 0.9.5
Comment 2 Darin Fisher 2001-09-28 15:15:52 PDT
Created attachment 51303 [details] [diff] [review]
patch: adds abstract base classes for nsHttpTransaction and nsHttpConnection
Comment 3 Darin Fisher 2001-09-28 15:17:25 PDT
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 4 Gagan 2001-09-28 16:40:10 PDT
Comment on attachment 51303 [details] [diff] [review]
patch: adds abstract base classes for nsHttpTransaction and nsHttpConnection

r=gagan
Comment 5 Darin Fisher 2001-10-01 17:28:19 PDT
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 6 rpotts (gone) 2001-10-01 17:30:44 PDT
Comment on attachment 51303 [details] [diff] [review]
patch: adds abstract base classes for nsHttpTransaction and nsHttpConnection

sr=rpotts@netscape.com
Comment 7 Darin Fisher 2001-10-01 17:35:38 PDT
OK... checked in the this patch on the trunk.
Comment 8 Darin Fisher 2001-10-01 17:37:17 PDT
-> 0.9.6 (for the actual pipelining implementation)
Comment 9 Darin Fisher 2001-11-08 15:45:14 PST
-> 0.9.9 (if time permits)
Comment 10 Darin Fisher 2001-12-21 17:50:21 PST
most likely a post mozilla 1.0 feature.
Comment 11 Darin Fisher 2002-03-11 19:23:25 PST
nope.. will work on a "disabled by default" solution for moz 1.0
Comment 12 Darin Fisher 2002-04-10 00:34:20 PDT
Created attachment 78516 [details] [diff] [review]
preliminary patch - works, but still needs a little tweaking

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.
Comment 13 Darin Fisher 2002-04-10 15:41:48 PDT
Created attachment 78629 [details]
HTTP log output (filtered) running 1 cycle of cowtools w/ pipelining enabled

 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.
Comment 14 Bradley Baetz (:bbaetz) 2002-04-10 15:52:35 PDT
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.
Comment 15 Darin Fisher 2002-04-10 16:51:56 PDT
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.
Comment 16 Darin Fisher 2002-04-10 17:45:52 PDT
Created attachment 78662 [details] [diff] [review]
revised patch - fixed some stability and reliability bugs

still needs some more tweaking.
Comment 17 Bradley Baetz (:bbaetz) 2002-04-10 17:59:19 PDT
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?
Comment 18 Bradley Baetz (:bbaetz) 2002-04-10 18:06:35 PDT
OOps - ignore that comment, made it on the wrong bug.
Comment 19 Darin Fisher 2002-04-11 13:41:40 PDT
Created attachment 78772 [details] [diff] [review]
v1 final

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).
Comment 20 Tomi Leppikangas 2002-04-15 14:27:27 PDT
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()) &&
Comment 21 Bradley Baetz (:bbaetz) 2002-04-15 15:34:41 PDT
Squid only claims to be HTTP/1.0, though, not HTTP/1.1
Comment 22 Gagan 2002-04-15 19:38:39 PDT
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)
Comment 23 Adam D. Moss 2002-04-18 02:52:22 PDT
Should the new file nsHttpPipeline.h be tri-licensed
instead of MPL?

(Rebuilding with the patch now...)
Comment 24 Adam D. Moss 2002-04-18 03:33:56 PDT
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.
Comment 25 Adam D. Moss 2002-04-18 03:45:29 PDT
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.
Comment 26 Darin Fisher 2002-04-18 15:19:09 PDT
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?
Comment 27 Adam D. Moss 2002-04-18 16:43:10 PDT
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.

Comment 28 Darin Fisher 2002-04-18 16:55:05 PDT
you can setup apache w/ mod_proxy, which should support pipelining.
Comment 29 Adam D. Moss 2002-04-18 17:02:51 PDT
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.
Comment 30 Darin Fisher 2002-04-18 17:16:42 PDT
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!
Comment 31 Harshal Pradhan 2002-04-19 00:17:11 PDT
In the constructor for nsHttpConnection. Don't you need a 

NS_INIT_ISUPPORTS();

Comment 32 rpotts (gone) 2002-04-19 01:18:45 PDT
Comment on attachment 78772 [details] [diff] [review]
v1 final

sr=rpotts@netscape.com

looks great!!
Comment 33 Harshal Pradhan 2002-04-19 01:22:42 PDT
Oops .. 
Of course, in the above post I meant nsHttpPipeline::nsHttpPipeline(). (and not 
nsHttpConnection)
Comment 34 Adam D. Moss 2002-04-19 04:59:47 PDT
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.
Comment 35 Adam D. Moss 2002-04-19 05:15:42 PDT
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.
Comment 36 AM 2002-04-19 05:28:30 PDT
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...
Comment 37 Adam D. Moss 2002-04-19 06:56:46 PDT
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.
Comment 38 Adam D. Moss 2002-04-19 07:05:16 PDT
Created attachment 79990 [details]
moz HTTP log while weirdness occurs with pipelining on.

gzipped for compactness.
Comment 39 Adam D. Moss 2002-04-19 07:10:38 PDT
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.
Comment 40 Darin Fisher 2002-04-19 10:50:05 PDT
Harshal: good catch!
Comment 41 Darin Fisher 2002-04-19 10:52:28 PDT
adam: thx for the log file!
Comment 42 Darin Fisher 2002-04-19 15:41:29 PDT
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!
Comment 43 Darin Fisher 2002-04-23 00:38:14 PDT
requires follow up patch in bug 138754.
Comment 44 Darin Fisher 2002-04-26 12:49:21 PDT
Created attachment 81194 [details] [diff] [review]
patch for mozilla 1.0 branch

no changes in this patch... it just includes the regression fix from bug
138754.
Comment 45 John Levon 2002-04-26 17:45:06 PDT
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 46 chris hofmann 2002-04-26 22:09:20 PDT
Comment on attachment 81194 [details] [diff] [review]
patch for mozilla 1.0 branch

a=chofmann off by default for the 1.0 branch.
Comment 47 scottputterman 2002-04-29 17:54:38 PDT
adding adt1.0.0+.  Please check into the branch as soon as possible and add the
fixed1.0.0 keyword.
Comment 48 Darin Fisher 2002-04-29 18:18:01 PDT
fixed-on-branch
Comment 49 Tom Everingham 2002-05-02 18:57:53 PDT
verified trunk and branch - http pipelining implemented and is off by default:
win NT4, linux rh6, mac osX
Comment 50 Adam D. Moss 2002-05-03 00:59:36 PDT
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.

Comment 51 Christian Mattar 2002-05-11 09:44:47 PDT
Shouldn't this project get marked as complete on
http://komodo.mozilla.org/planning/branches.cgi ?
Comment 52 benc 2002-09-10 14:01:33 PDT
forgot to remove fixed1.0.0 keyword so doing so now

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