822745 - implement spdy persistent cwnd (type) setting

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[Patrick McManus [:mcmanus]]

spdy has a mechanism as part of the settings frame where a server can inform the client of some network condition about the existing session so that the client can echo it back in the future rather than starting from scratch. it does that with the PERSIST_FLAG.

This patch implements the echoing of type 5 (CWND) in connection entry session state - no state is preserved between restarts. The server can use this to more accurately size the initial sending window than a clean connection would.

Server support for this on google.com is actually broken right now (the effect is a nop), but I've been working with that team and it will be repaired at the next google.com kernel refresh (unscheduled). We can pipeline this a little bit by getting the feature in.

Our support looks just like chrome's from a packet trace point of view.

To load gmail with a full cache it takes me 40 packets of data.. with an empty cache it takes more like 400. That's plenty of data flow to feel the effects of this.

Even with the small 40 packet case a CWND change to 30 (the most common value recorded by our user's telemetry) saves a round trip time and common values I saw when testing would save two of them. So that's on average a couple hundred ms improvement.

The one concern here is that this is really another server cookie mechanism and has privacy implications. It should have a privacy review before being enabled, which I will schedule. This patch includes the feature as turned off by default (controllable by a pref) until that is sorted out. We're effectively talking about a 7 bits of data that isn't persisted between restarts, so I'm overwhelemed by it.

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: patch 0

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 693524 [details] [diff] [review]
patch 0

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

::: netwerk/protocol/http/SpdySession3.cpp
@@ +636,5 @@
>  {
>    NS_ABORT_IF_FALSE(PR_GetCurrentThread() == gSocketThread, "wrong thread");
>    LOG3(("SpdySession3::GenerateSettings %p\n", this));
>  
> +  static const uint32_t maxDataLen = 20; // sized for 2 settings

instead of 12 please write it as 4 + 2 * 8.

@@ +1128,5 @@
>        self->mMaxConcurrent = value;
>        Telemetry::Accumulate(Telemetry::SPDY_SETTINGS_MAX_STREAMS, value);
>        break;
>        
> +    case SETTINGS_TYPE_CWND: 

whitespace

@@ +1135,5 @@
> +        nsRefPtr<nsHttpConnectionInfo> ci;
> +        self->GetConnectionInfo(getter_AddRefs(ci));
> +        if (ci)
> +          gHttpHandler->ConnMgr()->ReportSpdyCWNDSetting(ci, value);
> +        Telemetry::Accumulate(Telemetry::SPDY_SETTINGS_CWND, value);

And what if it is not be persisted?  Shouldn't we collect telemetry as well?

Comment 3

[Patrick McManus [:mcmanus]]

Attachment: patch 1

Honza, this is a very minor update to the version you r+'d. As part of the privacy review of the feature we decided to not echo this setting if it was more than 8 hours old. The patch reflects that change.

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 705512 [details] [diff] [review]
patch 1

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

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +595,5 @@
> +    // For privacy tracking reasons, and the fact that CWND is not
> +    // meaningful after some time, we don't honor stored CWND after 8
> +    // hours.
> +    TimeDuration age = TimeStamp::Now() - ent->mSpdyCWNDTimeStamp;
> +    if (age.ToMilliseconds() > (1000 * 60 * 60 * 8))

ToSeconds() is cheaper.

Comment 5

[Patrick McManus [:mcmanus]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/3bccc06c75b2

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3bccc06c75b2