Closed Bug 93054 Opened 23 years ago Closed 22 years ago

implement HTTP/1.1 pipelining

Categories

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

enhancement

Tracking

()

VERIFIED FIXED
mozilla1.0

People

(Reporter: darin.moz, Assigned: darin.moz)

References

Details

(Keywords: perf, topembed, Whiteboard: [adt2] [fixed-trunk])

Attachments

(4 files, 3 obsolete files)

implement HTTP/1.1 pipelining
Target Milestone: --- → mozilla1.0
Severity: normal → enhancement
QA Contact: benc → tever
Blocks: 95314
gonna try for 0.9.5
Target Milestone: mozilla1.0 → mozilla0.9.5
Depends on: 99562
Status: NEW → ASSIGNED
Priority: -- → P5
Depends on: 82873
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
Attachment #51303 - Flags: review+
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
Attachment #51303 - Flags: superreview+
OK... checked in the this patch on the trunk.
Attachment #51303 - Attachment is obsolete: true
-> 0.9.6 (for the actual pipelining implementation)
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Target Milestone: mozilla0.9.6 → mozilla0.9.7
-> 0.9.9 (if time permits)
Target Milestone: mozilla0.9.7 → mozilla0.9.9
most likely a post mozilla 1.0 feature.
Target Milestone: mozilla0.9.9 → mozilla1.0.1
Keywords: nsbeta1
nope.. will work on a "disabled by default" solution for moz 1.0
Keywords: nsbeta1nsbeta1+, perf
Target Milestone: mozilla1.0.1 → mozilla1.0
Priority: P5 → P3
Whiteboard: [adt2]
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.
Attachment #78516 - Attachment is obsolete: true
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.
Attached patch v1 finalSplinter Review
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).
Attachment #78662 - Attachment is obsolete: true
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)
Attachment #78772 - Flags: review+
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!!
Attachment #78772 - Flags: superreview+
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!
Whiteboard: [adt2] → [adt2] [fixed-trunk]
requires follow up patch in bug 138754.
Keywords: topembed
no changes in this patch... it just includes the regression fix from bug
138754.
Blocks: 138000
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.
Attachment #81194 - Flags: approval+
Keywords: adt1.0.0
adding adt1.0.0+.  Please check into the branch as soon as possible and add the
fixed1.0.0 keyword.
Keywords: adt1.0.0adt1.0.0+
fixed-on-branch
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Keywords: fixed1.0.0
Resolution: --- → FIXED
verified trunk and branch - http pipelining implemented and is off by default:
win NT4, linux rh6, mac osX
Status: RESOLVED → VERIFIED
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 ?
Keywords: verified1.0.0
forgot to remove fixed1.0.0 keyword so doing so now
Keywords: fixed1.0.0
Blocks: grouper
You need to log in before you can comment on or make changes to this bug.