Closed Bug 1340655 Opened 7 years ago Closed 7 years ago

Remove H1 Pipeline Support

Categories

(Core :: Networking: HTTP, defect)

50 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: mcmanus, Assigned: mcmanus)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [necko-active])

Attachments

(1 file)

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: nobody → mcmanus
Whiteboard: [necko-active]
honza's gonna like this one :)
Attachment #8838812 - Flags: review?(honzab.moz)
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
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.
(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.
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)
msvc complaining about a signed/unisnged mismatch. will confirm fix on try.
Flags: needinfo?(mcmanus)
https://hg.mozilla.org/mozilla-central/rev/fb3c9156f1b9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1344340
No longer depends on: 1344340
Blocks: 1347856
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.
Blocks: 1348930
You need to log in before you can comment on or make changes to this bug.