Remove H1 Pipeline Support

RESOLVED FIXED in Firefox 54

Status

()

Core
Networking: HTTP
RESOLVED FIXED
4 months ago
14 days ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

(Blocks: 2 bugs, {dev-doc-complete})

50 Branch
mozilla54
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(1 attachment)

(Assignee)

Description

4 months ago
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
(Assignee)

Comment 1

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ee110bd1042
Assignee: nobody → mcmanus
Whiteboard: [necko-active]
(Assignee)

Comment 2

4 months ago
honza's gonna like this one :)
(Assignee)

Comment 3

4 months ago
Created attachment 8838812 [details] [diff] [review]
remove h1 pipeline support
Attachment #8838812 - Flags: review?(honzab.moz)
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.
Blocks: 1337826

Updated

4 months ago
Keywords: dev-doc-needed
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?
Attachment #8838812 - Flags: review?(honzab.moz) → review+
Blocks: 1312774
(Assignee)

Comment 6

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

Comment 7

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

Comment 8

4 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/379093669b392384fffab147a1e7b42e835bceee
Bug 1340655 - remove h1 pipeline support r=mayhemer
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
Flags: needinfo?(mcmanus)
(Assignee)

Comment 10

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b749969f34f
(Assignee)

Comment 11

4 months ago
msvc complaining about a signed/unisnged mismatch. will confirm fix on try.
Flags: needinfo?(mcmanus)
(Assignee)

Comment 12

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

Comment 13

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fb3c9156f1b9
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Updated

4 months ago
Depends on: 1344340
(Assignee)

Updated

4 months ago
No longer depends on: 1344340

Updated

3 months ago
Blocks: 1347856

Comment 14

3 months ago
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?
(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.

Updated

3 months ago
Blocks: 1348930
Tidied up the doc changes previously made to https://developer.mozilla.org/en-US/docs/Mozilla/Preferences/Mozilla_networking_preferences.
Keywords: dev-doc-needed → dev-doc-complete
Blocks: 1366202
You need to log in before you can comment on or make changes to this bug.