1340655 - Remove H1 Pipeline Support

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

I tried so hard to make h1 pipelining a reality. In practice, it has been subsumed by the more effective mux of spdy, h2, and soon quic.

The feature was never able to overcome the limitations of HoL blocking and its impact on performance - fundamentally mux is the only way we should be approaching that problem.

The massive amount of code - mostly heuristics to try and deal with the HoL problem (by classifying things onto different transactions, timers for broken servers, etc..) has become a maintenance burden with minimal value given the default off state of the feature.

we should delete it and embrace h2 as the new (working) strategy.

I'll make a patch

Comment 1

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ee110bd1042

Comment 2

[Patrick McManus [:mcmanus]]

honza's gonna like this one :)

Comment 3

[Patrick McManus [:mcmanus]]

Attachment: remove h1 pipeline support

Comment 4

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

We might want to update this MDN (https://developer.mozilla.org/en-US/docs/Mozilla/Preferences/Mozilla_networking_preferences) as well after this patch is landed.

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 8838812 [details] [diff] [review]
remove h1 pipeline support

Review of attachment 8838812 [details] [diff] [review]:
-----------------------------------------------------------------

Except one condition in nsHttpConnection::CanReuse() that is a bit disputable this seems OK.

r=honzab with that addressed or explained.

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +176,5 @@
>    NS_IMETHOD GetOriginalResponseHeader(const nsACString &aHeader,
>                                         nsIHttpHeaderVisitor *aVisitor) override;
>    NS_IMETHOD VisitOriginalResponseHeaders(nsIHttpHeaderVisitor *aVisitor) override;
> +  NS_IMETHOD GetAllowPipelining(bool *value) override; // deprecated
> +  NS_IMETHOD SetAllowPipelining(bool value) override;  // deprecated

any plans to remove it from the idl?  or you don't want to break any js addons that may call on it?

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ -118,5 @@
>                         a.referrerPolicy(), a.apiRedirectTo(), a.topWindowURI(),
>                         a.loadFlags(), a.requestHeaders(),
>                         a.requestMethod(), a.uploadStream(),
>                         a.uploadStreamHasHeaders(), a.priority(), a.classOfService(),
> -                       a.redirectionLimit(), a.allowPipelining(), a.allowSTS(),

allowPipelining should also be removed from the ipdl params (NeckoChannelParams.ipdlh)

::: netwerk/protocol/http/nsHttp.h
@@ +50,5 @@
>  // http connection capabilities
>  //-----------------------------------------------------------------------------
>  
>  #define NS_HTTP_ALLOW_KEEPALIVE      (1<<0)
> +// NS_HTTP_ALLOW_PIPELINING          (1<<1) removed

I think we can just remove this?

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +783,1 @@
>          return false;

when you are here, if () { }

but, the the old condition could be (by parts) written as:

if (!mTransaction)
  if (0 >= mRemainingConnectionUses /* uint */)
     return false;

hence can be reduced to:

if (!mTransaction && !mRemainingConnectionUses) 
  return false;


the other part then: 

if (mTransaction) 
  if (mTransaction->IsDone()) /* PipelineDepth = IsDone ? 1 : 0 */
    if (1 >= mRemainingConnectionUses)
      return false;
  else
    if (!mRemaningConnectionsUses)
      return false;


or am I wrong?  anyway, I think this is not the same as just !mRemainingConnectionUses -> return false.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +1275,5 @@
>      }
>  
>      // step 1
>      // If connection pressure, then we want to favor pipelining of any kind
> +    // h1 pipelining has been removed

maybe remove the comment mentioning pipelining altogether?  up to you.

@@ +1339,5 @@
>      }
>  
>      // step 3
>      // consider pipelining scripts and revalidations
> +    // h1 pipelining has been removed

as well here

@@ +1485,5 @@
>  
>      // give the transaction the indirect reference to the connection.
>      transaction->SetConnection(handle);
>  
> +    nsresult rv = conn->Activate(transaction, caps, priority);

please move nsresult rv as the first declaration in the method.

::: netwerk/test/unit/xpcshell.ini
@@ -89,5 @@
>  [test_URIs2.js]
>  # Intermittent time-outs on Android, bug 1285020
>  requesttimeoutfactor = 2
>  [test_aboutblank.js]
> -[test_assoc.js]

is this the right test to remove?

Comment 6

[Patrick McManus [:mcmanus]]

Thanks for the review!

> 
> ::: netwerk/protocol/http/HttpBaseChannel.h
> 
> any plans to remove it from the idl?  or you don't want to break any js
> addons that may call on it?

didn't want to break anything. "SetAllow..()" never forced pipelining anyhow, so there is no semantic change there worth breaking over.

> 
> allowPipelining should also be removed from the ipdl params
> (NeckoChannelParams.ipdlh)

good catch - thank you; I've made the change.
 
> I think we can just remove this?

when stuff like that is non-contiguous I like to just make it clear its now unused. feel free to reuse the bit though :) This is also true of the comments in ConnectionMgr where some dispatch steps are now unused.. I left them in as nops just as a convenience to people who are accustomed to the way it was structured.

> 
> ::: netwerk/protocol/http/nsHttpConnection.cpp
> @@ +783,1 @@
> >          return false;
>


re nsHttpConnection::CanReuse() - I think you're right. Basically I think you're saying that without pipelnes I was reducing PipeineDepth() to 0, when it is really (IsDone() ? 0 : 1). I'll make that change.


> > -[test_assoc.js]
> 
> is this the right test to remove?

yes. that test made sure the right responses where matched up with the right pipelined requests.

Comment 7

[Patrick McManus [:mcmanus]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #4)
> We might want to update this MDN
> (https://developer.mozilla.org/en-US/docs/Mozilla/Preferences/
> Mozilla_networking_preferences) as well after this patch is landed.

thanks sc. I've updated the page with the reference to it being dropped in 54. I will land after the trees open.

Comment 8

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/379093669b392384fffab147a1e7b42e835bceee
Bug 1340655 - remove h1 pipeline support r=mayhemer

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for warning as err.or at nsHttpConnection.cpp(875):

https://hg.mozilla.org/integration/mozilla-inbound/rev/665c5023212598462d33d7addc1e14d7c7c10ee3

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=379093669b392384fffab147a1e7b42e835bceee&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=80941095&repo=mozilla-inbound

19:13:51     INFO -  z:/build/build/src/sccache2/sccache.exe z:/build/build/src/vs2015u3/VC/bin/amd64_x86/cl.exe -FoUnified_cpp_dom_base5.obj -c -Iz:/build/build/src/obj-firefox/dist/stl_wrappers  -DNDEBUG=1 -DTRIMMED=1 -DWIN32_LEAN_AND_MEAN -D_WIN32 -DWIN32 -D_CRT_RAND_S -DCERT_CHAIN_PARA_HAS_EXTRA_FIELDS -DOS_WIN=1 -D_UNICODE -DCHROMIUM_BUILD -DU_STATIC_IMPLEMENTATION -DUNICODE -D_WINDOWS -D_SECURE_ATL -DCOMPILER_MSVC -DHAVE_SIDEBAR -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -Iz:/build/build/src/dom/base -Iz:/build/build/src/obj-firefox/dom/base -Iz:/build/build/src/dom/battery -Iz:/build/build/src/dom/events -Iz:/build/build/src/dom/media -Iz:/build/build/src/dom/network -Iz:/build/build/src/dom/time -Iz:/build/build/src/caps -Iz:/build/build/src/docshell/base -Iz:/build/build/src/dom/base -Iz:/build/build/src/dom/file -Iz:/build/build/src/dom/geolocation -Iz:/build/build/src/dom/html -Iz:/build/build/src/dom/ipc -Iz:/build/build/src/dom/storage -Iz:/build/build/src/dom/svg -Iz:/build/build/src/dom/u2f -Iz:/build/build/src/dom/workers -Iz:/build/build/src/dom/xbl -Iz:/build/build/src/dom/xml -Iz:/build/build/src/dom/xslt/xpath -Iz:/build/build/src/dom/xul -Iz:/build/build/src/gfx/2d -Iz:/build/build/src/image -Iz:/build/build/src/js/xpconnect/src -Iz:/build/build/src/js/xpconnect/wrappers -Iz:/build/build/src/layout/base -Iz:/build/build/src/layout/forms -Iz:/build/build/src/layout/generic -Iz:/build/build/src/layout/style -Iz:/build/build/src/layout/svg -Iz:/build/build/src/layout/xul -Iz:/build/build/src/netwerk/base -Iz:/build/build/src/security/manager/ssl -Iz:/build/build/src/widget -Iz:/build/build/src/xpcom/ds -Iz:/build/build/src/netwerk/sctp/datachannel -Iz:/build/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -Iz:/build/build/src/ipc/chromium/src -Iz:/build/build/src/ipc/glue -Iz:/build/build/src/obj-firefox/dist/include  -Iz:/build/build/src/obj-firefox/dist/include/nspr -Iz:/build/build/src/obj-firefox/dist/include/nss        -MD -FI z:/build/build/src/obj-firefox/mozilla-config.h -DMOZILLA_CLIENT -deps.deps/Unified_cpp_dom_base5.obj.pp  -TP -nologo -wd5026 -wd5027 -Zc:sizedDealloc- -wd4091 -wd4577 -D_HAS_EXCEPTIONS=0 -W3 -Gy -Zc:inline -utf-8 -arch:SSE2 -FS -Gw -wd4251 -wd4244 -wd4267 -wd4800 -wd4595 -we4553 -GR-  -Z7 -O1 -Oi -Oy- -WX    z:/build/build/src/obj-firefox/dom/base/Unified_cpp_dom_base5.cpp
19:13:51     INFO -  Unified_cpp_protocol_http1.cpp
19:13:51     INFO -  z:/build/build/src/netwerk/protocol/http/nsHttpConnection.cpp(875): error C2220: warning treated as error - no 'object' file generated
19:13:51     INFO -  Warning: C4018 in z:\build\build\src\netwerk\protocol\http\nsHttpConnection.cpp: '>=': signed/unsigned mismatch
19:13:51     INFO -  z:/build/build/src/netwerk/protocol/http/nsHttpConnection.cpp(875): warning C4018: '>=': signed/unsigned mismatch
19:13:51     INFO -  z:/build/build/src/config/rules.mk:1014: recipe for target 'Unified_cpp_protocol_http1.obj' failed
19:13:51     INFO -  mozmake.EXE[5]: *** [Unified_cpp_protocol_http1.obj] Error 2

Comment 10

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b749969f34f

Comment 11

[Patrick McManus [:mcmanus]]

msvc complaining about a signed/unisnged mismatch. will confirm fix on try.

Comment 12

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb3c9156f1b99c4b08e7c35b98ac27756c357687
Bug 1340655 - remove h1 pipeline support r=mayhemer

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fb3c9156f1b9

Comment 14

[Nomis101]

I think you have forgotten to remove Pipeline here: https://dxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#78
and here: https://dxr.mozilla.org/mozilla-central/source/mobile/android/app/mobile.js#102
Should I fill a follow-up for this?

Comment 15

[Dragana Damjanovic [:dragana]]

(In reply to Nomis101 from comment #14)
> I think you have forgotten to remove Pipeline here:
> https://dxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#78
> and here:
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/app/mobile.
> js#102
> Should I fill a follow-up for this?

the second one is fixed in inbound and will be fixed in m.-c. tomorrow. You can file the bug for the first one, but that is not that important.

Comment 16

[Eric Shepherd [:sheppy]]

Tidied up the doc changes previously made to https://developer.mozilla.org/en-US/docs/Mozilla/Preferences/Mozilla_networking_preferences.