93054 - implement HTTP/1.1 pipelining

Status: VERIFIED FIXED

(Core :: Networking: HTTP, enhancement, P3)

Description

[Darin Fisher]

implement HTTP/1.1 pipelining

Comment 1

[Darin Fisher]

gonna try for 0.9.5

Comment 2

[Darin Fisher]

Attachment: patch: adds abstract base classes for nsHttpTransaction and nsHttpConnection

Comment 3

[Darin Fisher]

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]

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

r=gagan

Comment 5

[Darin Fisher]

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)]

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

sr=[e-mail]

Comment 7

[Darin Fisher]

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

Comment 8

[Darin Fisher]

-> 0.9.6 (for the actual pipelining implementation)

Comment 9

[Darin Fisher]

-> 0.9.9 (if time permits)

Comment 10

[Darin Fisher]

most likely a post mozilla 1.0 feature.

Comment 11

[Darin Fisher]

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

Comment 12

[Darin Fisher]

Attachment: 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]

Attachment: 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)]

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]

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]

Attachment: revised patch - fixed some stability and reliability bugs

still needs some more tweaking.

Comment 17

[Bradley Baetz (:bbaetz)]

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)]

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

Comment 19

[Darin Fisher]

Attachment: 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]

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)]

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

Comment 22

[Gagan]

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]

Should the new file nsHttpPipeline.h be tri-licensed
instead of MPL?

(Rebuilding with the patch now...)

Comment 24

[Adam D. Moss]

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]

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]

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]

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]

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

Comment 29

[Adam D. Moss]

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]

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]

In the constructor for nsHttpConnection. Don't you need a 

NS_INIT_ISUPPORTS();

Comment 32

[rpotts (gone)]

Comment on attachment 78772 [details] [diff] [review]
v1 final

sr=[e-mail]

looks great!!

Comment 33

[Harshal Pradhan]

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

Comment 34

[Adam D. Moss]

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]

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]

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]

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]

Attachment: moz HTTP log while weirdness occurs with pipelining on.

gzipped for compactness.

Comment 39

[Adam D. Moss]

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]

Harshal: good catch!

Comment 41

[Darin Fisher]

adam: thx for the log file!

Comment 42

[Darin Fisher]

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]

requires follow up patch in bug 138754.

Comment 44

[Darin Fisher]

Attachment: patch for mozilla 1.0 branch

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

Comment 45

[John Levon]

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]

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]

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

Comment 48

[Darin Fisher]

fixed-on-branch

Comment 49

[Tom Everingham]

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

Comment 50

[Adam D. Moss]

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]

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

Comment 52

[benc]

forgot to remove fixed1.0.0 keyword so doing so now