Closed Bug 822745 Opened 7 years ago Closed 7 years ago

implement spdy persistent cwnd (type) setting

Categories

(Core :: Networking: HTTP, enhancement)

x86_64
Linux
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: mcmanus, Assigned: mcmanus)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch patch 0 (obsolete) — Splinter Review
Attachment #693524 - Flags: review?(honzab.moz)
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?
Attachment #693524 - Flags: review?(honzab.moz) → review+
Depends on: 822790
Attached patch patch 1Splinter Review
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.
Attachment #693524 - Attachment is obsolete: true
Attachment #705512 - Flags: review?(honzab.moz)
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.
Attachment #705512 - Flags: review?(honzab.moz) → review+
https://hg.mozilla.org/mozilla-central/rev/3bccc06c75b2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.