64169 2011-07-08 07:56:09 -0700 [Qt] Enable Http Pipelining by default 2011-07-13 04:42:58 -0700 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED Qt, QtTriaged P2 Normal --- 64352 64448 1 benjamin benjamin ademar hausmann kling laszlo.gombos markus s.mathur oldest_to_newest 434053 0 benjamin 2011-07-08 07:56:09 -0700 Http pipelining is disabled by default on QNetworkAccessManager. We need to set QNetworkRequest::HttpPipeliningAllowedAttribute on the network request. 434056 1 100120 benjamin 2011-07-08 08:18:02 -0700 Created attachment 100120 Patch 434063 2 100120 kling 2011-07-08 08:31:41 -0700 Comment on attachment 100120 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. 434077 3 benjamin 2011-07-08 08:50:56 -0700 Committed r90634: <http://trac.webkit.org/changeset/90634> 434093 4 benjamin 2011-07-08 09:05:56 -0700 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. 434098 5 s.mathur 2011-07-08 09:13:36 -0700 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. 434127 6 ademar 2011-07-08 10:18:01 -0700 (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). 435056 7 ademar 2011-07-11 11:45:19 -0700 Removing from 2.2. Please reply to my previous comment if you disagree with the reasoning. :) 435598 8 markus 2011-07-12 05:46:39 -0700 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 ... 435604 9 kling 2011-07-12 06:14:45 -0700 (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? 435605 10 markus 2011-07-12 06:24:53 -0700 (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? 435610 11 kling 2011-07-12 06:33:10 -0700 (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 435611 12 markus 2011-07-12 06:35:07 -0700 (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. 435614 13 s.mathur 2011-07-12 06:37:42 -0700 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. 435615 14 markus 2011-07-12 06:45:17 -0700 (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. 100120 2011-07-08 08:18:02 -0700 2011-07-08 08:31:41 -0700 Patch bug-64169-20110708173024.patch text/plain 1643 benjamin U3VidmVyc2lvbiBSZXZpc2lvbjogOTA2MzEKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBkN2RiMWM3ZmNjZGMyZTFk N2NmZDM2YzdlMDNlMjM4N2UwOWQ4ZGJlLi44MjEwZGU3N2EzNTFkM2VmNTU0MDZmNDAwMDgzNjNm OWQ2YmY0MGUxIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTEtMDctMDggIEJlbmph bWluIFBvdWxhaW4gIDxiZW5qYW1pbkB3ZWJraXQub3JnPgorCisgICAgICAgIFtRdF0gRW5hYmxl IEh0dHAgUGlwZWxpbmluZyBieSBkZWZhdWx0CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD02NDE2OQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgorCisgICAgICAgIFFOZXR3b3JrQWNjZXNzTWFuYWdlciBkaXNhYmxlIEhUVFAgcGlw ZWxpbmluZyBieSBkZWZhdWx0LiBXZSBlbmFibGUgaXQgYnkKKyAgICAgICAgc2V0dGluZyBhbiBh dHRyaWJ1dGUgb24gdGhlIHJlcXVlc3QuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9uZXR3b3JrL3F0 L1Jlc291cmNlUmVxdWVzdFF0LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlJlc291cmNlUmVxdWVz dDo6dG9OZXR3b3JrUmVxdWVzdCk6CisKIDIwMTEtMDctMDggIEFsZXhhbmRlciBQYXZsb3YgIDxh cGF2bG92QGNocm9taXVtLm9yZz4KIAogICAgICAgICBXZWIgSW5zcGVjdG9yOiBDU1MgaW5zcGVj dG9yIGdldHMgY29uZnVzZWQgYWJvdXQgc3BlY2lmaWNpdHkgb2YgIWltcG9ydGFudCBwcm9wZXJ0 aWVzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9uZXR3b3JrL3F0L1Jlc291 cmNlUmVxdWVzdFF0LmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsvcXQvUmVz b3VyY2VSZXF1ZXN0UXQuY3BwCmluZGV4IDU0YzA4ZDA1NzBhYjU4OTUwNzJkZjFlYjBkNjhjMDM1 MTJkMDgyNzkuLmRlNjBiZDE0ZDBiYjcwZGI0Njg2NWZmOTEwNjBmZDIyOTg5M2YzM2YgMTAwNjQ0 Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsvcXQvUmVzb3VyY2VSZXF1ZXN0 UXQuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsvcXQvUmVzb3VyY2VS ZXF1ZXN0UXQuY3BwCkBAIC00OSw2ICs0OSw3IEBAIFFOZXR3b3JrUmVxdWVzdCBSZXNvdXJjZVJl cXVlc3Q6OnRvTmV0d29ya1JlcXVlc3QoUU9iamVjdCogb3JpZ2luYXRpbmdGcmFtZSkgY29uCiAg ICAgUU5ldHdvcmtSZXF1ZXN0IHJlcXVlc3Q7CiAgICAgcmVxdWVzdC5zZXRVcmwodXJsKCkpOwog ICAgIHJlcXVlc3Quc2V0T3JpZ2luYXRpbmdPYmplY3Qob3JpZ2luYXRpbmdGcmFtZSk7CisgICAg cmVxdWVzdC5zZXRBdHRyaWJ1dGUoUU5ldHdvcmtSZXF1ZXN0OjpIdHRwUGlwZWxpbmluZ0FsbG93 ZWRBdHRyaWJ1dGUsIHRydWUpOwogCiAgICAgY29uc3QgSFRUUEhlYWRlck1hcCAmaGVhZGVycyA9 IGh0dHBIZWFkZXJGaWVsZHMoKTsKICAgICBmb3IgKEhUVFBIZWFkZXJNYXA6OmNvbnN0X2l0ZXJh dG9yIGl0ID0gaGVhZGVycy5iZWdpbigpLCBlbmQgPSBoZWFkZXJzLmVuZCgpOwo=