The default bug view has changed. See this FAQ.

implement HTTP/1.1 pipelining

VERIFIED FIXED in mozilla1.0

Status

()

Core
Networking: HTTP
P3
enhancement
VERIFIED FIXED
16 years ago
24 days ago

People

(Reporter: Darin Fisher, Assigned: Darin Fisher)

Tracking

({perf, topembed})

Trunk
mozilla1.0
perf, topembed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2] [fixed-trunk])

Attachments

(4 attachments, 3 obsolete attachments)

(Assignee)

Description

16 years ago
implement HTTP/1.1 pipelining
(Assignee)

Updated

16 years ago
Target Milestone: --- → mozilla1.0

Updated

16 years ago
Severity: normal → enhancement
QA Contact: benc → tever
(Assignee)

Updated

16 years ago
Blocks: 95314
(Assignee)

Comment 1

16 years ago
gonna try for 0.9.5
Target Milestone: mozilla1.0 → mozilla0.9.5
(Assignee)

Updated

16 years ago
Depends on: 99562
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
Priority: -- → P5
(Assignee)

Updated

16 years ago
Depends on: 82873
(Assignee)

Comment 2

16 years ago
Created attachment 51303 [details] [diff] [review]
patch: adds abstract base classes for nsHttpTransaction and nsHttpConnection
(Assignee)

Comment 3

16 years ago
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

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

r=gagan
Attachment #51303 - Flags: review+
(Assignee)

Comment 5

16 years ago
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

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

sr=rpotts@netscape.com
Attachment #51303 - Flags: superreview+
(Assignee)

Comment 7

16 years ago
OK... checked in the this patch on the trunk.
(Assignee)

Updated

16 years ago
Attachment #51303 - Attachment is obsolete: true
(Assignee)

Comment 8

16 years ago
-> 0.9.6 (for the actual pipelining implementation)
Target Milestone: mozilla0.9.5 → mozilla0.9.6
(Assignee)

Updated

16 years ago
Target Milestone: mozilla0.9.6 → mozilla0.9.7
(Assignee)

Comment 9

16 years ago
-> 0.9.9 (if time permits)
Target Milestone: mozilla0.9.7 → mozilla0.9.9
(Assignee)

Comment 10

16 years ago
most likely a post mozilla 1.0 feature.
Target Milestone: mozilla0.9.9 → mozilla1.0.1

Updated

15 years ago
Keywords: nsbeta1
(Assignee)

Comment 11

15 years ago
nope.. will work on a "disabled by default" solution for moz 1.0
Keywords: nsbeta1 → nsbeta1+, perf
Target Milestone: mozilla1.0.1 → mozilla1.0
(Assignee)

Updated

15 years ago
Priority: P5 → P3

Updated

15 years ago
Whiteboard: [adt2]
(Assignee)

Comment 12

15 years ago
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.
(Assignee)

Comment 13

15 years ago
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.
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.
(Assignee)

Comment 15

15 years ago
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.
(Assignee)

Comment 16

15 years ago
Created attachment 78662 [details] [diff] [review]
revised patch - fixed some stability and reliability bugs

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.
(Assignee)

Comment 19

15 years ago
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).
Attachment #78662 - Attachment is obsolete: true

Comment 20

15 years ago
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 22

15 years ago
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+

Comment 23

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

(Rebuilding with the patch now...)

Comment 24

15 years ago
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

15 years ago
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.
(Assignee)

Comment 26

15 years ago
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

15 years ago
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.

(Assignee)

Comment 28

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

Comment 29

15 years ago
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.
(Assignee)

Comment 30

15 years ago
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

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

NS_INIT_ISUPPORTS();

Comment 32

15 years ago
Comment on attachment 78772 [details] [diff] [review]
v1 final

sr=rpotts@netscape.com

looks great!!
Attachment #78772 - Flags: superreview+

Comment 33

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

Comment 34

15 years ago
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

15 years ago
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

15 years ago
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

15 years ago
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

15 years ago
Created attachment 79990 [details]
moz HTTP log while weirdness occurs with pipelining on.

gzipped for compactness.

Comment 39

15 years ago
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.
(Assignee)

Comment 40

15 years ago
Harshal: good catch!
(Assignee)

Comment 41

15 years ago
adam: thx for the log file!
(Assignee)

Comment 42

15 years ago
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]
(Assignee)

Comment 43

15 years ago
requires follow up patch in bug 138754.
(Assignee)

Updated

15 years ago
Keywords: topembed
(Assignee)

Comment 44

15 years ago
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.

Updated

15 years ago
Blocks: 138000

Comment 45

15 years ago
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

15 years ago
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+
(Assignee)

Updated

15 years ago
Keywords: adt1.0.0

Comment 47

15 years ago
adding adt1.0.0+.  Please check into the branch as soon as possible and add the
fixed1.0.0 keyword.
Keywords: adt1.0.0 → adt1.0.0+
(Assignee)

Comment 48

15 years ago
fixed-on-branch
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Keywords: fixed1.0.0
Resolution: --- → FIXED

Comment 49

15 years ago
verified trunk and branch - http pipelining implemented and is off by default:
win NT4, linux rh6, mac osX
Status: RESOLVED → VERIFIED

Comment 50

15 years ago
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

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

Updated

15 years ago
Keywords: verified1.0.0

Comment 52

15 years ago
forgot to remove fixed1.0.0 keyword so doing so now
Keywords: fixed1.0.0

Updated

15 years ago
Blocks: 176349
You need to log in before you can comment on or make changes to this bug.