Bug 64169 - [Qt] Enable Http Pipelining by default
Summary: [Qt] Enable Http Pipelining by default
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords: Qt, QtTriaged
Depends on: 64352 64448
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-08 07:56 PDT by Benjamin Poulain
Modified: 2011-07-13 04:42 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.60 KB, patch)
2011-07-08 08:18 PDT, Benjamin Poulain
kling: review+
kling: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2011-07-08 07:56:09 PDT
Http pipelining is disabled by default on QNetworkAccessManager. We need to set QNetworkRequest::HttpPipeliningAllowedAttribute on the network request.
Comment 1 Benjamin Poulain 2011-07-08 08:18:02 PDT
Created attachment 100120 [details]
Patch
Comment 2 Andreas Kling 2011-07-08 08:31:41 PDT
Comment on attachment 100120 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100120&action=review

r=me

> Source/WebCore/ChangeLog:3
> +        [Qt] Enable Http Pipelining by default

Http -> HTTP

> Source/WebCore/ChangeLog:8
> +        QNetworkAccessManager disable HTTP pipelining by default. We enable it by

disable -> disables

> Source/WebCore/platform/network/qt/ResourceRequestQt.cpp:52
> +    request.setAttribute(QNetworkRequest::HttpPipeliningAllowedAttribute, true);

HTTP pipelining is per request? wat.
Comment 3 Benjamin Poulain 2011-07-08 08:50:56 PDT
Committed r90634: <http://trac.webkit.org/changeset/90634>
Comment 4 Benjamin Poulain 2011-07-08 09:05:56 PDT
Laszlo, I did not put it has a dependency for 2.2 because it probably has side effects. I think you should check with the network team before adding that a stable branch.
Comment 5 Siddharth Mathur 2011-07-08 09:13:36 PDT
It's OK to make this ON by default for 2.2. All of Nokia's in-the-wild mobile browsers have had this for a long time, with no ills. The tunable parameters are already OK in QtNetwork.
Comment 6 Ademar Reis 2011-07-08 10:18:01 PDT
(In reply to comment #5)
> It's OK to make this ON by default for 2.2. All of Nokia's in-the-wild mobile browsers have had this for a long time, with no ills. The tunable parameters are already OK in QtNetwork.

I think we should not assume that this is so simple. From http://en.wikipedia.org/wiki/HTTP_pipelining#Implementation_in_web_browsers:

"""
HTTP pipelining is disabled in most browsers[3].

    * Opera has pipelining enabled by default. It uses heuristics to control the level of pipelining employed depending on the connected server.[4]
    * Internet Explorer 8 does not pipeline requests, due to concerns regarding buggy proxies and head-of-line blocking.[5]
    * Mozilla browsers (such as Mozilla Firefox, SeaMonkey and Camino), support pipelining however it is disabled by default.[6][7] It uses some heuristics, especially to turn pipelining off for IIS servers.[8]
    * Konqueror 2.0 supports pipelining, but it's disabled by default.[citation needed]
    * Google Chrome does not support pipelining.[9]
"""

Based on that, I don't think we should enable it by default on our stable branch, specially after declaring the beta (trunk will have more time for regression tests, so it may be worth the risk).
Comment 7 Ademar Reis 2011-07-11 11:45:19 PDT
Removing from 2.2. Please reply to my previous comment if you disagree with the reasoning. :)
Comment 8 Markus Goetz 2011-07-12 05:46:39 PDT
It's unfortunately not that easy.

You cannot enable pipelining for all requests because some requests might clog the socket. An example is long lives HTTP connections for HTTP polling or COMET or however you might call it.
All requests "after" that in the pipeline will then not be processed until the poll has finished.

So you should not enable pipelining for Ajax and general script stuff.

See for example bug 50644 ...
Comment 9 Andreas Kling 2011-07-12 06:14:45 PDT
(In reply to comment #8)
> It's unfortunately not that easy.
> 
> You cannot enable pipelining for all requests because some requests might clog the socket. An example is long lives HTTP connections for HTTP polling or COMET or however you might call it.
> All requests "after" that in the pipeline will then not be processed until the poll has finished.
> 
> So you should not enable pipelining for Ajax and general script stuff.

Fascinating. How did we not run into issues with this on the in-the-wild browsers currently using this?
Comment 10 Markus Goetz 2011-07-12 06:24:53 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > So you should not enable pipelining for Ajax and general script stuff.
> 
> Fascinating. How did we not run into issues with this on the in-the-wild browsers currently using this?

There are browsers using this?
Comment 11 Andreas Kling 2011-07-12 06:33:10 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > So you should not enable pipelining for Ajax and general script stuff.
> > 
> > Fascinating. How did we not run into issues with this on the in-the-wild browsers currently using this?
> 
> There are browsers using this?

See comment #5
Comment 12 Markus Goetz 2011-07-12 06:35:07 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (In reply to comment #8)
> > > > So you should not enable pipelining for Ajax and general script stuff.
> > > 
> > > Fascinating. How did we not run into issues with this on the in-the-wild browsers currently using this?
> > 
> > There are browsers using this?
> 
> See comment #5

That comment says nothing about if the current browsers use it for Ajax or not.
Pipelining should be done. But you should not use it for all kind of requests.
Comment 13 Siddharth Mathur 2011-07-12 06:37:42 PDT
I checked yesterday with RS Subramanian (Symbian HTTP and networking layer guru) and he mentioned that native Symbian browser (v7.x) uses upto 2 pipelined HTTP requests. He said that using more exposed many problems in remote web-servers as well as in the client-side HTTP/TCP stack, so a not-so-aggressive value was decided upon. 

Therefore, I suggest that we do _not_ turn on for the 2.2 branch, and let the downstream users figure it out.
Comment 14 Markus Goetz 2011-07-12 06:45:17 PDT
(In reply to comment #13)
> Therefore, I suggest that we do _not_ turn on for the 2.2 branch, and let the downstream users figure it out.

The plan is to have it as a default in QNetworkAccessManager at some point.
http://bugreports.qt.nokia.com/browse/QTBUG-19052

However it makes sense to decouple this from Qt's future and QtWebKit could do it therefore in trunk (but not 2.2) as soon as there's some code to have it not be done for Ajax.