796192 - A/B test HTTP Pipelining on pre-release channel

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Valentin Gosu [:valentin] (he/him)]

Temporarily enable HTTP pipelining and evaluate whether it offers a significant performance improvement.

Comment 1

[Valentin Gosu [:valentin] (he/him)]

Hi Patrick,

What is the policy for enabling HTTP pipelining?
* enable by default
* enable for 10 minutes for each browsing session
* enable for the first X thousand connections for each session

Also, how do we deal with the prefs? Do we just enable/disable the prefs? How about people who are keen on keeping it on/off?

How do we evaluate if this is a successful experiment or not?

Comment 2

[Patrick McManus [:mcmanus]]

(In reply to Valentin Gosu from comment #1)
> Hi Patrick,
> 
> What is the policy for enabling HTTP pipelining?

it needs its own pref and should be dependent on existing network.allow-experiments pref and should only apply to a small fraction of sessions (~1% ??) and should only last for the first few (10?) minutes of those sessions..

>
> Also, how do we deal with the prefs? Do we just enable/disable the prefs?

no - you'll just override the prefs.

> How about people who are keen on keeping it on/off?

see "own pref" above.

> How do we evaluate if this is a successful experiment or not?

using the queue time telemetry you added earlier.

Actually, I looked at that data today and it was not good for pipelines vs normal http (but it as fabulous for spdy). So we might not be ready to turn this on yet (or maybe its just because it needs a broader sample - I need to look harder at it) but having this mechanism in-tree lets us do that eval when we're ready. so this work can proceed but we might leave the pref off at first.

Comment 3

[Valentin Gosu [:valentin] (he/him)]

Attachment: Patch 1.0

Added network.http.pipelining.testrun=true by default
Behavior:
If network.allow-experiments is true, then when the pref is changed to true (or the browser starts), the pipelining is enabled, and a timer is set for 10 mins. If the pref is turned off, or the timer expires, the pipeline option is set to the original value of network.http.pipelining

Comment 4

[Patrick McManus [:mcmanus]]

Comment on attachment 666861 [details] [diff] [review]
Patch 1.0

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

::: modules/libpref/src/init/all.js
@@ +865,5 @@
>  pref("network.http.accept-encoding", "gzip, deflate");
>  
>  pref("network.http.pipelining"      , false);
>  pref("network.http.pipelining.ssl"  , false); // disable pipelining over SSL
> +pref("network.http.pipelining.testrun", true);

please name this abtest

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +39,5 @@
>  
>  #include "nsIXULAppInfo.h"
>  
>  #include "mozilla/net/NeckoChild.h"
> +#include <stdio.h>

why?

@@ +185,5 @@
>      LOG(("Creating nsHttpHandler [this=%x].\n", this));
>  
>      NS_ASSERTION(!gHttpHandler, "HTTP handler already created!");
>      gHttpHandler = this;
> +    mPipelineTestTimer = nullptr;

the safe pointer type inits that for you so you don't need that

@@ +186,5 @@
>  
>      NS_ASSERTION(!gHttpHandler, "HTTP handler already created!");
>      gHttpHandler = this;
> +    mPipelineTestTimer = nullptr;
> +    mPipeliningEnabled = false;

put it in the normal initializer list

@@ +1196,5 @@
>  
> +    //
> +    // Test HTTP Pipelining (bug796192)
> +    // If experiments are allowed and pipelining is Off, 
> +    // turn it On for just 10 minutes

you missed the part about only applying to 1% (maybe 1/128) of sessions. Get32BitsOfPseudoRandom() will be helpful.

@@ +1228,5 @@
>  #undef MULTI_PREF_CHANGED
>  }
>  
> +void
> +nsHttpHandler::TimerCallback(nsITimer * aTimer, void * aClosure)

document what timer is being called

@@ +1232,5 @@
> +nsHttpHandler::TimerCallback(nsITimer * aTimer, void * aClosure)
> +{
> +    nsRefPtr<nsHttpHandler> thisObject = static_cast<nsHttpHandler*>(aClosure);
> +    if (!thisObject->mPipeliningEnabled)
> +        thisObject->mCapabilities &= ~NS_HTTP_ALLOW_PIPELINING;            

trailing whitespace

Comment 5

[icecold.spam]

Attachment: Patch 2.0

Comment 6

[Valentin Gosu [:valentin] (he/him)]

Oops. I was logged in with the wrong account. That's my patch actually :)

Comment 7

[Patrick McManus [:mcmanus]]

Comment on attachment 667226 [details] [diff] [review]
Patch 2.0

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

almost there! thanks.

::: modules/libpref/src/init/all.js
@@ +865,5 @@
>  pref("network.http.accept-encoding", "gzip, deflate");
>  
>  pref("network.http.pipelining"      , false);
>  pref("network.http.pipelining.ssl"  , false); // disable pipelining over SSL
> +pref("network.http.pipelining.abtest", true);

false.. not ready to turn this on yet (see comment 1)

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +1462,5 @@
>          httpChannel = new nsHttpChannel();
>      }
>  
>      uint8_t caps = mCapabilities;
> +    if (mPipeliningABTest && (rand() % 128 == 0))

use the pseudorandom funciton in http handler.. then you don't have to worry about whether or not it has been initialized.

but more importantly this randomly selects channels.. we need to randomly select sessions (i.e. at startup (or pref change - that's fine)) and then do all the channels within the session for 10 minutes.

mixing and matching channels would give funky interdependent results.

Comment 8

[Valentin Gosu [:valentin] (he/him)]

(In reply to Patrick McManus [:mcmanus] from comment #7)
> >      uint8_t caps = mCapabilities;
> > +    if (mPipeliningABTest && (rand() % 128 == 0))
> 
> use the pseudorandom funciton in http handler.. then you don't have to worry
> about whether or not it has been initialized.
> 

I used the rand() because Get32BitsOfPseudoRandom() needs to be on the socket thread. Neither PrefsChanged() or NewProxiedChannel() is called from the socket thread.

> but more importantly this randomly selects channels.. we need to randomly
> select sessions (i.e. at startup (or pref change - that's fine)) and then do
> all the channels within the session for 10 minutes.
> 
> mixing and matching channels would give funky interdependent results.

You're right, that makes a lot of sense.
I'll just go back to the previous method, by placing the following in the proper branch of PrefsChanged()

if (cVar && !(rand() % 128)) {
                mCapabilities |=  NS_HTTP_ALLOW_PIPELINING;

From my tests it seems that rand has been properly seeded at this point.
Hope that's ok.

Comment 9

[Valentin Gosu [:valentin] (he/him)]

Attachment: Patch 3.0

Comment 10

[Patrick McManus [:mcmanus]]

Comment on attachment 667559 [details] [diff] [review]
Patch 3.0

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

great. Thanks for this. put it through (1 platform) try before checking in.

Comment 11

[Valentin Gosu [:valentin] (he/him)]

Attachment: Patch 3.0 checkin-needed

https://tbpl.mozilla.org/?tree=Try&rev=2eb4ef98cde1

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 668217 [details] [diff] [review]
Patch 3.0 checkin-needed

checkin-needed is all you need

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d750568367fe

Comment 14

[Valentin Gosu [:valentin] (he/him)]

Thanks!

Comment 15

[Ed Morley [:emorley]]

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