737470 - spdy v3

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[Patrick McManus [:mcmanus]]

v3 largely for

* flow control
* server push (now that we have flow control)

we'll provide a short overlap of supporting both v2 and v3, but pre-standardization we won't be providing old versions in perpituity. chrome is taking the same approach.

* abstract a spdy class so that v2 an v3 can be in separate, largely duplicate, c++ files.Take this approach during standardization rather than intermingling code as http/1.1 v http/1.0 to allow rapid iteration. post standardization might be a differnet story if we get to a point where we want more than 1 version.

Comment 1

[Patrick McManus [:mcmanus]]

proxy support too (726366)

Comment 2

[Patrick McManus [:mcmanus]]

General Changelog:

* Implement a split from 1 Spdy implementation to 2. We'll try and maintain the most recent 2 throughout standardization. I intentionally didn't make this N, that's a matrix we don't want.

* change "x-firefox-spdy: 1" response header to be "x-firefox-spdy: [protocol version used]"

* telemetry for which version was used

* separate prefs for v2 and v3

Changes specific to v3:

* Remove NOOP frame

* Updated version field in transmits and expected in receives

* The header block used by syn-stream, syn-reply, and headers now uses 32 bit length values for strings and the pair count

* updated the compression dictionary as per spec. Also removed v2 workaround that included the null at the end of the dictionary

* removed the little endian workaround of the settings frame values from v2.

* updated syn_stream to use an extra priority bit and changed our mapping of nsISupportsPriority to reflect that, also to make sure that -10, and -11 get different spdy priority values as that seems to be a common differentiation in gecko.

* added the client cert slot id to syn_stream, but kept it at 0 (which means always use the TLS client cert).

* updated syn_reply to reflect removal of unused 2 byte field

* updated the list of RST codes

* implementation of RST_STREAM_IN_USE and RST_STREAM_ALREADY_CLOSED that had more general codes in v2

* Split the spdy request header "url" into :scheme :host and :path

* Move other spdy specific headers (both request and response) to escaped colon notation

* Implement reciept of HEADER frames in between SYN_REPLY and DATA

* Flow control for both upstream and downstream using WINDOW_UPDATE and SETTINGS INITIAL_WINDOW. This also resulted in the removal of the "urgent queue", as it made more sense to service the necessary window updates directly out of SpdySession to avoid the queuing problems.

Comment 3

[Patrick McManus [:mcmanus]]

I've implemented this as 3 patches:

* 737470.patch-1.split-spdy adds a general ASpdySession layer that nsHttpConnection* can interact with to negotiate the correct spdy version. This means updates to new revisions of spdy won't significantly change nsHttp* code. At this patch, ASpdySession still only speaks v2 but that is opaque to nsHttp*

737470.patch-2.clone-v2 clones v2 into v3 and makes ASpdySession able to negotiate v3. If you build at this patch and actually negotiate v3 with a server things will fail because the clone "v3" implementation is really just v2 in a new class. I've done that so the real spdy3 specific changes in the next patch are more clear. (and v3 is preffed off here).

737470.patch-3.spdy3 - these are the real spdy3 specific changes from v2. (and v3 is preffed on)

Comment 4

[Patrick McManus [:mcmanus]]

(In reply to Patrick McManus [:mcmanus] from comment #2)

> * Implement reciept of HEADER frames in between SYN_REPLY and DATA
> 

its worth noting that this means other spdy frames can be interleaved into what was previously data all in the v2 syn-reply. That means the decompression buffer and logic needed to be moved into the stream class instead of the session class - which wasn't that big of a deal but results in a significant part of the diff.

Comment 5

[Patrick McManus [:mcmanus]]

Attachment: patch 1 rev 0 - split into 2 spdy revisions

Comment 6

[Patrick McManus [:mcmanus]]

Attachment: patch 2 rev 0 - clone v2 into v3

Comment 7

[Patrick McManus [:mcmanus]]

Attachment: patch 3 rev 0 - spdy3

Comment 8

[Patrick McManus [:mcmanus]]

Attachment: patch 1 rev 1 - split into 2 spdy revisions

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 620011 [details] [diff] [review]
patch 1 rev 1 - split into 2 spdy revisions

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

::: netwerk/protocol/http/ASpdySession.h
@@ +80,5 @@
> +  // determine if the primary version is enabled or preffed off
> +  bool Protocol0Enabled();
> +
> +  // determine if the backup version is enabled or preffed off
> +  bool Protocol1Enabled();

Rather make this taking an index as well: ProtocolEnabled(int index).  It may help using this API on some places (cycles etc).

::: netwerk/protocol/http/SpdySession.h
@@ +257,2 @@
>    nsClassHashtable<nsPtrHashKey<nsAHttpTransaction>,
> +                   SpdyStream2>                        mStreamTransactionHash;

indention

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +323,5 @@
> +    if (!negotiatedNPN.IsEmpty()) {
> +        if (negotiatedNPN.Equals(gHttpHandler->SpdyInfo()->VersionString[0]))
> +            StartSpdy(gHttpHandler->SpdyInfo()->Version[0]);
> +        else if (negotiatedNPN.Equals(gHttpHandler->SpdyInfo()->VersionString[1]))
> +            StartSpdy(gHttpHandler->SpdyInfo()->Version[1]);

Could SpdyInfo have a method that returns the version number index (0 or 1) based on the npn string?

@@ +481,5 @@
> +        (gHttpHandler->SpdyInfo()->Protocol1Enabled() &&
> +         nsHttp::FindToken(val,
> +                           gHttpHandler->SpdyInfo()->AlternateProtocolString[1].get(),
> +                           HTTP_HEADER_VALUE_SEPS))) {
> +         LOG(("Connection %p Transaction %p found Alternate-Protocol "

Could there be SpdyInfo::CheckAlternateProtocolString method that checks all versions we provide or takes an index as an arg?

::: netwerk/protocol/http/nsHttpConnection.h
@@ +264,5 @@
>      bool                            mNPNComplete;
>      bool                            mSetupNPNCalled;
> +
> +    // version level in use, 0 if unused
> +    PRUint8                         mUsingSpdy;

mUsingSpdyVersion

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +213,5 @@
>      , mAllowExperiments(true)
>      , mEnableSpdy(false)
>      , mCoalesceSpdy(true)
>      , mUseAlternateProtocol(false)
> +    , mSpdySendingChunkSize(4096)

This is a step backward to not use kXXX.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +801,5 @@
>    NS_ASSERTION(NS_SUCCEEDED(rv),
>                 "GetNegotiatedNPN() failed during renegotiation");
>  
> +  if (NS_SUCCEEDED(rv) && !StringBeginsWith(negotiatedNPN,
> +                                            NS_LITERAL_CSTRING("spdy/")))

Rather check separately for both supported versions, but up to you.

::: toolkit/components/telemetry/TelemetryHistograms.h
@@ +199,5 @@
>  
>  HTTP_HISTOGRAMS(PAGE, "page: ")
>  HTTP_HISTOGRAMS(SUB, "subitem: ")
>  
> +HISTOGRAM(SPDY_VERSION, 1, 16, 16, LINEAR, "SPDY: Protocol Version Used")

I think it should be 1, 16, 1 or just 1, 2, 1.  Worth to do a check on this.

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 619682 [details] [diff] [review]
patch 2 rev 0 - clone v2 into v3

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

The new files for Spdy 3 are just copies or already has some new implementation?

I assume they are the same files.  Then I'd rather make them with hg cp.  We may need to look easily back to even v2 history when fixing regressions in v3.

::: netwerk/protocol/http/ASpdySession.cpp
@@ +69,5 @@
>      
> +  if (version == SpdyInformation::SPDY_VERSION_2)
> +    return new SpdySession2(aTransaction, aTransport, aPriority);
> +
> +  return new SpdySession3(aTransaction, aTransport, aPriority);

switch?

@@ +74,5 @@
>  }
>  
>  SpdyInformation::SpdyInformation()
>  {
> +// list the preferred version first

indent

Comment 11

[Honza Bambas (:mayhemer)]

Comment on attachment 619682 [details] [diff] [review]
patch 2 rev 0 - clone v2 into v3

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

r=honzab but please use hg cp to create the new files.

Comment 12

[Patrick McManus [:mcmanus]]

awesome honza.

fyi I think I plan to update the patches for nits and then check them in with v3 turned off in ff15 and then flip the pref when we transition to ff16 in early june to maximize the testing window as google.com now serves v3.

Comment 13

[Patrick McManus [:mcmanus]]

> > +HISTOGRAM(SPDY_VERSION, 1, 16, 16, LINEAR, "SPDY: Protocol Version Used")
> 
> I think it should be 1, 16, 1 or just 1, 2, 1.  Worth to do a check on this.

The idea is that over time the versions will range from 2 to ?? (hopefully < 16). 

so 1, 16, 16 means min=1, max=16, with 16 buckets so each version can be tracked independently.

It seems better to just set it up for the dataset we expect to see than adjusting histograms.h for every version.

Comment 14

[Patrick McManus [:mcmanus]]

Comment on attachment 619683 [details] [diff] [review]
patch 3 rev 0 - spdy3

honza this is on the quarterly goal list, so I wanted to ping you on it.

I have kept it current with the other changes to existing spdy2 support.

Comment 15

[Patrick McManus [:mcmanus]]

Attachment: patch 1 rev 2 - split into 2 spdy revisions

updated just for review comments and bitrot

Comment 16

[Patrick McManus [:mcmanus]]

Attachment: patch 2 rev 1 - clone v2 into v3

updated for review comments and bitrot.

Comment 17

[Patrick McManus [:mcmanus]]

Attachment: patch 3 rev 1 - spdy3

update for bitrot

Comment 18

[Honza Bambas (:mayhemer)]

Please don't update since now.  I'll start reviewing the part 3 soon.

Comment 19

[Honza Bambas (:mayhemer)]

I started the review of part3 patch, so please don't update it.


How is this new code being tested?  I will not r+ the patch w/o having as much as possible reasonably good tests for it.

Comment 20

[Patrick McManus [:mcmanus]]

(In reply to Honza Bambas (:mayhemer) from comment #19)

>
> How is this new code being tested?  I will not r+ the patch w/o having as
> much as possible reasonably good tests for it.

you can test it against google.com, netty or https://www.spdybook.com/

chris strom is updating node-spdy for v3, and when that's done we can add it to the testbase.

Comment 21

[Honza Bambas (:mayhemer)]

(In reply to Patrick McManus [:mcmanus] from comment #20)
> chris strom is updating node-spdy for v3, and when that's done we can add it
> to the testbase.

What bug is that?

Comment 22

[Patrick McManus [:mcmanus]]

(In reply to Honza Bambas (:mayhemer) from comment #21)
> (In reply to Patrick McManus [:mcmanus] from comment #20)
> > chris strom is updating node-spdy for v3, and when that's done we can add it
> > to the testbase.
> 
> What bug is that?

his work isn't a mozilla thing (its code we import), so there is no bug. If you'd like a bug for updating testbase then I guess it needs to be filed.

Comment 23

[Honza Bambas (:mayhemer)]

Comment on attachment 626191 [details] [diff] [review]
patch 3 rev 1 - spdy3

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

sorry to say that, but your coding style is extremely hard to read and the state logic hard to follow.  That applies to version 2 as well.  During this review I didn't check all code paths and state changes.  It would be just a waste of time, I believe tests will cover that at least partially.  I'm also sad you didn't encapsulate the buffering logic to some class or rather turned to using our streams before you forked to version 3.

please remove comments "// In v3 this is where we would generate a window update"

::: netwerk/protocol/http/SpdySession3.cpp
@@ +1070,4 @@
>        self->mDownstreamRstReason == RST_FLOW_CONTROL_ERROR) {
>      // basically just ignore this
>      self->ResetDownstreamState();
>      return NS_OK;

Add a log.

@@ +1172,5 @@
> +        self->mServerInitialWindow = value;
> +
> +        // we need to add the delta to all open streams (delta can be negative)
> +        self->mStreamTransactionHash.Enumerate(UpdateServerRwinEnumerator,
> +                                               &delta);

You must not update streams that have already received WINDOW_UPDATE.  That would break the flow control, right?  Simplest is IMO to just don't update at all on any existing stream.

"7 - SETTINGS_INITIAL_WINDOW_SIZE allows the sender to inform the remote endpoint the initial window size (in bytes) for *new* streams."

@@ +1347,5 @@
> +    return NS_ERROR_ILLEGAL_VALUE;
> +  }
> +
> +  PRUint32 delta =
> +    PR_ntohl(reinterpret_cast<PRUint32 *>(self->mInputFrameBuffer.get())[3]);

Mask with 0x7fffffff.

@@ +1366,5 @@
> +    self->ResetDownstreamState();
> +    return NS_OK;
> +  }
> +
> +  PRInt64 remoteWindow = self->mInputFrameDataStream->RemoteWindow();

s/remoteWindow/lastRemoteWindow/

@@ +1373,5 @@
> +  LOG3(("SpdySession3::HandleWindowUpdate %p stream 0x%X window "
> +        "%d increased by %d.\n", self, streamID, remoteWindow, delta));
> +
> +  // If the stream had a 0 window, then schedule it for writing again
> +  if (!remoteWindow) {

And what about negative values?

@@ +1525,5 @@
> +  if (stream->BlockedOnRwin()) {
> +    LOG3(("SpdySession3 %p will stream %p 0x%x suspended for flow control\n",
> +          this, stream, stream->StreamID()));
> +    rv = NS_BASE_STREAM_WOULD_BLOCK;
> +    return rv;

Just return?

@@ +1739,5 @@
>        // This will happen when the transaction figures out it is EOF, generally
>        // due to a content-length match being made
>        SpdyStream3 *stream = mInputFrameDataStream;
> +      // if we were doing header completion about that
> +      mDownstreamState = PROCESSING_DATA_FRAME;

I don't understand the comment.  Please explain better why you reset the state.

@@ +1861,5 @@
> +  // Don't necessarily ack every data packet. Only do it if
> +  // after a significant amount of data.
> +  PRUint64 unacked = stream->LocalUnAcked();
> +
> +  if (unacked < kMinimumToAck) {

You are comparing PRUint64 and PRUint32.

@@ +1871,5 @@
> +  }
> +
> +  // Generate window updates directly out of spdysession instead of the stream
> +  // in order to avoid queue delays in getting the ACK out.
> +  PRUint32 toack = unacked & 0xffffffff;

Very interesting code.  

Correct would be PRUint32 toack = (PRUint32)NS_MAX<PRInt64>(unacked, 0x7fffffff);

probably use toack to compare with kMinumumToAck.

Also, when we have more then 2^32 unacked (impossible to happen IMO from few reasons), you have to send WINDOW_UPDATE with 0x7fffffff for so many times to ack all pending data on our side.  So this code is incomplete.

@@ +1895,5 @@
> +  toack = PR_htonl(toack);
> +  memcpy(packet + 12, &toack, 4);
> +
> +  LogIO(this, stream, "Window Update", packet, 8 + dataLen);
> +  FlushOutputQueue();

Hmm.. but you actually didn't read the data.  In other words, how can you be sure there is an RWin or your side?

Does it make sense to send the update out when mInputFrameDataLast ?

@@ +2151,5 @@
>            this, caller));
>      return;
>    }
>    
> +  LOG3(("SpdySession3::TransactionHasDataToWrite %p ID is 0x%x\n",

Can you please be consistent with how you log things?  StreamID is on some places logged as 0x%X, here it is as 0x%x.  It may then be harder to filter based on stream ID.

::: netwerk/protocol/http/SpdySession3.h
@@ +156,5 @@
> +  // until we have an API that can push back on receiving data (right now
> +  // WriteSegments is obligated to accept data and buffer) there is no
> +  // reason to throttle with the rwin other than in server push
> +  // scenarios.
> +  const static PRUint32 kInitialRwin = 256 * 1024 * 1024;

Where did you take this number from?

@@ +224,5 @@
>    void        ProcessPending();
>    nsresult    SetInputFrameDataStream(PRUint32);
>    bool        VerifyStream(SpdyStream3 *, PRUint32);
>  
> +  void        UpdateLocalRwin(SpdyStream3 *stream,PRUint32 bytes);

space between args

::: netwerk/protocol/http/SpdyStream3.cpp
@@ +99,3 @@
>    mTxInlineFrame = new char[mTxInlineFrameSize];
> +  mDecompressBuffer = new char[mDecompressBufferSize];
> +

Remove this blank line

@@ +152,5 @@
>      // for writing
>      if (rv == NS_BASE_STREAM_WOULD_BLOCK && !mTxInlineFrameUsed)
>        mRequestBlockedOnRead = 1;
>  
> +    if (!mBlockedOnRwin &&

Add a comment why this has been added to the condition.

@@ +355,5 @@
> +    // transactions. 
> +    PRUint8 calculatedPriority = 3 + ((mPriority + 1) / 5);
> +    NS_ABORT_IF_FALSE (!(calculatedPriority & 0xf8),
> +                       "Calculated Priority Out Of Range");
> +    mTxInlineFrame[16] = calculatedPriority << 5;

Mask this to be sure we are in the range and the |Unused| part is just zeros.

@@ +917,5 @@
> +    if (zlib_rv == Z_DATA_ERROR || zlib_rv == Z_MEM_ERROR)
> +      return NS_ERROR_FAILURE;
> +
> +    mDecompressBufferUsed += mDecompressBufferSize - mDecompressBufferUsed -
> +      context->avail_out;

For those not familiar with zlib API please document here that |avail_out| is decreased by the amount zlib has put to the output buffer.

@@ +951,5 @@
> +  for (PRUint32 index = 0; index < numPairs; ++index) {
> +    if (lastHeaderByte < nvpair + 4)
> +      return NS_ERROR_ILLEGAL_VALUE;
> +    PRUint32 nameLen = (nvpair[0] << 24) + (nvpair[1] << 16) +
> +      (nvpair[2] << 8) + nvpair[3];

Why is numPairs OK to ntohl and here you do it "manually"?

@@ +1108,5 @@
> +      }
> +
> +      aHeadersOut.Append(NS_LITERAL_CSTRING("\r\n"));
> +    }
> +    nvpair += 8 + nameLen + valueLen;

This all code is so hard to read and so untrustworthy...

::: netwerk/protocol/http/SpdyStream3.h
@@ +97,5 @@
>  
> +  nsresult Uncompress(z_stream *, char *, PRUint32);
> +  nsresult ConvertHeaders(nsACString &);
> +
> +  void UpdateRemoteWindow(PRInt32 delta) { mRemoteWindow += delta; }

Hmm.. some overflow checking may be wise here.  Actually overflow would mean a flow control violation.

@@ +197,5 @@
>  
>    // Flag is set after the WAITING_FOR Transport event has been generated
>    PRUint32                     mSentWaitingFor       : 1;
>  
> +  // Flag is set after 1 DATA frame has been passed to stream, after

1st DATA ?

@@ +236,5 @@
>    PRInt32                      mPriority;
>  
> +  // For mLocalWindow, mRemoteWindow, and mLocalUnacked are for flow control.
> +  // *window are signed because they race conditions in asynchronous SETTINGS
> +  // messages can force them temporarily negative.

Check wording please.

@@ +249,5 @@
> +
> +  // LocalUnacked is the number of bytes received by the client but not
> +  //   yet reflected in a window update. Sending that update will increment
> +  //   LocalWindow
> +  PRUint64                     mLocalUnacked;

What is the exact reason to keep these as 64-bit?  

The limits fit 32-bits.  

Delta of the local window can only be up to 24 bits, and we don't buffer now more then 32-bit length.  Also, you are sending WINDOW_UPDATE when we have more then 64k unacked, so the unacked value will never go over 2^24.

Server window can be up to 32-bits.  It cannot send WINDOW_UPDATEs for more then what we have sent, and according the flow control logic we cannot send more then 32-bits of length of data before we get the first WINDOW_UPDATE.

@@ +251,5 @@
> +  //   yet reflected in a window update. Sending that update will increment
> +  //   LocalWindow
> +  PRUint64                     mLocalUnacked;
> +
> +  bool                         mBlockedOnRwin;

And this one is?

Comment 24

[Patrick McManus [:mcmanus]]

> 
> @@ +1172,5 @@
> > +        self->mServerInitialWindow = value;
> > +
> > +        // we need to add the delta to all open streams (delta can be negative)
> > +        self->mStreamTransactionHash.Enumerate(UpdateServerRwinEnumerator,
> > +                                               &delta);
> 
> You must not update streams that have already received WINDOW_UPDATE.  That
> would break the flow control, right?  Simplest is IMO to just don't update
> at all on any existing stream.
> 
> "7 - SETTINGS_INITIAL_WINDOW_SIZE allows the sender to inform the remote
> endpoint the initial window size (in bytes) for *new* streams."

no.. though this is a bit of a sticky wicket.

consider a brand new connection. The server desires a window size of 32KB so it sends a settings with an IW-SIZE of 32KB immediately.

on the client side we don't wait for that settings to arrive - we don't even know that its coming. So we send off a SYN_STREAM of "POST /". What should be the window size for that stream?

Well it depends on whether we read that settings frame first or not, right? Assuming not - initially it's 64KB, which does mean we can over send. The spec has language that talks about this race condition. But it shouldn't stay at 64KB for the lifetime of the stream when we know that the baseline number the server wants is really 32KB - thus the update via Enumerate().


> @@ +1366,5 @@
> > +    self->ResetDownstreamState();
> > +    return NS_OK;
> > +  }
> > +
> > +  PRInt64 remoteWindow = self->mInputFrameDataStream->RemoteWindow();
> 
> s/remoteWindow/lastRemoteWindow/
> 

I went with oldRemoteWindow

> @@ +1373,5 @@
> > +  LOG3(("SpdySession3::HandleWindowUpdate %p stream 0x%X window "
> > +        "%d increased by %d.\n", self, streamID, remoteWindow, delta));
> > +
> > +  // If the stream had a 0 window, then schedule it for writing again
> > +  if (!remoteWindow) {
> 
> And what about negative values?

this is a good point. thanks!


> 
> @@ +1861,5 @@
> > +  // Don't necessarily ack every data packet. Only do it if
> > +  // after a significant amount of data.
> > +  PRUint64 unacked = stream->LocalUnAcked();
> > +
> > +  if (unacked < kMinimumToAck) {
> 
> You are comparing PRUint64 and PRUint32.
> 

is there a problem with that? the 32bit should be promoted correctly for the comparison.

> @@ +1871,5 @@
> > +  }
> > +
> > +  // Generate window updates directly out of spdysession instead of the stream
> > +  // in order to avoid queue delays in getting the ACK out.
> > +  PRUint32 toack = unacked & 0xffffffff;
> 
> Very interesting code.  
> 
> Correct would be PRUint32 toack = (PRUint32)NS_MAX<PRInt64>(unacked,
> 0x7fffffff);
> 

it should be 31 bits (as in your text) not 32 as in mine. I'll fix that.

But as for the syntax I prefer my version honestly. It clearly takes the low bits of unacked. The proposed alternative is a mess of type casts and macros.

> probably use toack to compare with kMinumumToAck.
> 
> Also, when we have more then 2^32 unacked (impossible to happen IMO from few
> reasons), you have to send WINDOW_UPDATE with 0x7fffffff for so many times
> to ack all pending data on our side.  So this code is incomplete.

As you say, that can't really happen for a bunch of reasons. And if it does happen, this code is consistent - it will ack as much as it can (31 bits work), and update the internal state correctly. When it gets another data packet it will ack more. That seems fine for a case that can't happen if everything is in spec.


> @@ +1895,5 @@
> > +  toack = PR_htonl(toack);
> > +  memcpy(packet + 12, &toack, 4);
> > +
> > +  LogIO(this, stream, "Window Update", packet, 8 + dataLen);
> > +  FlushOutputQueue();
> 
> Hmm.. but you actually didn't read the data.  In other words, how can you be
> sure there is an RWin or your side?
> 

I'm not really sure what you're asking. Are you concerned that there will be a problem with generating the ack based on the data frame header instead of the actual data? If the data doesn't follow then that's a protocol error that will be detected in due course.

if we were really managing finite buffers here, the way flow control is probably used on the server, the data would add to the unacked value but we would never generate a window_update until some buffers were freed up. but in our case that lag would just needlessly slow things down - the idea here is keep that window open so data keeps flowing.

I do have some interest in using flow control in the client - but more in a stop/start way than managing a whole stream with a finite set of buffers. For example - a server pushed stream should be limited to fairly small amount of data until we get a request transaction to pair up with the pushed stream... and eventually you can envision media controls "pausing" a stream in this way. but for normal http gateway stuff we want to basically turn it off.

> Does it make sense to send the update out when mInputFrameDataLast ?

I've got that covered.. see the check for stream->RecvdFin() at the top of updatelocalrwin()

> ::: netwerk/protocol/http/SpdySession3.h
> @@ +156,5 @@
> > +  // until we have an API that can push back on receiving data (right now
> > +  // WriteSegments is obligated to accept data and buffer) there is no
> > +  // reason to throttle with the rwin other than in server push
> > +  // scenarios.
> > +  const static PRUint32 kInitialRwin = 256 * 1024 * 1024;
> 
> Where did you take this number from?

as the comment implies - it is designed to be huge and effectively disable downstream flow control. Also, at 2^28 it is comfortably far away from 2^31 that it probably won't edge test anyones server implementation. I'm open to other numbers but frankly everything from 2^30 down to 2^22 or so seem pretty equivalent. Below that and I worry about needlessly impinging on extreme BDPs. You'll see at http://japhr.blogspot.com/2012/05/spdy3-flow-control-comparisons.html that chrome makes that (imo) mistake with its 2^16 window.

> 
> ::: netwerk/protocol/http/SpdyStream3.cpp

> 
> @@ +355,5 @@
> > +    // transactions. 
> > +    PRUint8 calculatedPriority = 3 + ((mPriority + 1) / 5);
> > +    NS_ABORT_IF_FALSE (!(calculatedPriority & 0xf8),
> > +                       "Calculated Priority Out Of Range");
> > +    mTxInlineFrame[16] = calculatedPriority << 5;
> 
> Mask this to be sure we are in the range and the |Unused| part is just zeros.

the unused part is the lower 5 bits of (PRUint8 << 5) right? by definition those are zeros, it doesn't need another mask.


> @@ +951,5 @@
> > +  for (PRUint32 index = 0; index < numPairs; ++index) {
> > +    if (lastHeaderByte < nvpair + 4)
> > +      return NS_ERROR_ILLEGAL_VALUE;
> > +    PRUint32 nameLen = (nvpair[0] << 24) + (nvpair[1] << 16) +
> > +      (nvpair[2] << 8) + nvpair[3];
> 
> Why is numPairs OK to ntohl and here you do it "manually"?

numPairs is word aligned whereas nvpair may not be because it is offset by the actual lengths of the names and values.

> 
> @@ +249,5 @@
> > +
> > +  // LocalUnacked is the number of bytes received by the client but not
> > +  //   yet reflected in a window update. Sending that update will increment
> > +  //   LocalWindow
> > +  PRUint64                     mLocalUnacked;
> 
> What is the exact reason to keep these as 64-bit?  
> 
> The limits fit 32-bits.  
>

that's true... even with extreme constants. otoh when working with very large numbers I am using more and more 64 bit types. unless you're in an inner loop they perform fine on any platform (and on many platforms perform fine there too) and they simply prevent most overflow bugs at very little cost except perhaps if you have large arrays of them. So is there a compelling reason to change this?

Comment 25

[Patrick McManus [:mcmanus]]

Attachment: patch 3 rev 2 - spdy3

Comment 26

[Honza Bambas (:mayhemer)]

> > @@ +1895,5 @@
> > > +  toack = PR_htonl(toack);
> > > +  memcpy(packet + 12, &toack, 4);
> > > +
> > > +  LogIO(this, stream, "Window Update", packet, 8 + dataLen);
> > > +  FlushOutputQueue();
> > 
> > Hmm.. but you actually didn't read the data.  In other words, how can you be
> > sure there is an RWin or your side?
> > 
> 
> I'm not really sure what you're asking. Are you concerned that there will be
> a problem with generating the ack based on the data frame header instead of
> the actual data? If the data doesn't follow then that's a protocol error
> that will be detected in due course.
> 
> if we were really managing finite buffers here, the way flow control is
> probably used on the server, the data would add to the unacked value but we
> would never generate a window_update until some buffers were freed up. but
> in our case that lag would just needlessly slow things down - the idea here
> is keep that window open so data keeps flowing.
> 
> I do have some interest in using flow control in the client - but more in a
> stop/start way than managing a whole stream with a finite set of buffers.
> For example - a server pushed stream should be limited to fairly small
> amount of data until we get a request transaction to pair up with the pushed
> stream... and eventually you can envision media controls "pausing" a stream
> in this way. but for normal http gateway stuff we want to basically turn it
> off.

Our actual receive window at this level is 786432 bytes.  That is the pipe buffer size of a transaction used to consume data.  However, that is not what we should use to calculate probably.

If you simply don't want to use flow control from our side, you probably have a reason.  I'm just curious what the behavior is going to be - whether better w/o or w/ FC.  We'll see.  I cannot say at the moment.  It needs experimentation.

> 
> > Does it make sense to send the update out when mInputFrameDataLast ?
> 
> I've got that covered.. see the check for stream->RecvdFin() at the top of
> updatelocalrwin()

No... you set that flag later in ResetDownstreamState() if I follow the code correctly.  Just check on that.

> http://japhr.blogspot.com/2012/05/spdy3-flow-control-comparisons.html that
> chrome makes that (imo) mistake with its 2^16 window.

Interesting data, thanks.

> the unused part is the lower 5 bits of (PRUint8 << 5) right? by definition
> those are zeros, it doesn't need another mask.

Ah, right.  More caffeine.

> numPairs is word aligned whereas nvpair may not be because it is offset by
> the actual lengths of the names and values.

Does it actually matter?

Comment 27

[Patrick McManus [:mcmanus]]

(In reply to Honza Bambas (:mayhemer) from comment #26)

> 
> Our actual receive window at this level is 786432 bytes.  That is the pipe
> buffer size of a transaction used to consume data.  However, that is not
> what we should use to calculate probably.

right - that's our buffer. The rwin for the spdy stream needs to be that PLUS the bandwidth delay product of the connection, because it needs to cover all the data in flight in order to keep the pipe full. How big is BDP? That's a really hard question and it constantly changes.

If you guess too small you artificially slow down the transfer in high latency environments. Wasting bandwidth is an anti-goal :)

If you guess too big (which I have no doubt done for pretty much any BDP known to the Internet) you get head of line blocking of other streams if your local buffers get saturated. I'm guessing that our local buffers don't actually get saturated.. maybe I'll find a minute and try and file some telemetry about that. Even when you are doing HOL blocking it doesn't matter if none of the other streams would progress if they weren't blocked (e.g. a jpeg decoder loop gets really intense and the transaction buffer backs all the way up.. that does block other streams but if they can't run because the main thread is occupied then it doesn't much matter.)

In the standards effort I am advocating changing this to a xon/xoff scheme, so that we wouldn't have to a priori configure a window size that covered an unknowable BDP. Instead if we had our local buffers backing up on a per transaction basis we could issue a per stream xoff to stop the sending at that point. We'd have to be a little flexible on our buffer limit (becuase we'd still get another BDP's worth of data), but that would work much better because it doesn't require sizing things with BDP in mind. We'll see if I can get the Internet to agree with me :)

> 
> > 
> > > Does it make sense to send the update out when mInputFrameDataLast ?
> > 
> > I've got that covered.. see the check for stream->RecvdFin() at the top of
> > updatelocalrwin()
> 
> No... you set that flag later in ResetDownstreamState() if I follow the code
> correctly.  Just check on that.
>

good point!

> 
> > numPairs is word aligned whereas nvpair may not be because it is offset by
> > the actual lengths of the names and values.
> 
> Does it actually matter?

its always mattered on some risc architectures (which throw faults at you when you do unaligned 32 bit accesses).. I avoid it out of habit and compatibility goodness after spending a few years developing on dec alphas. I'm unsure if it matters in a gecko context. In a inner loop I'd study it carefully but the work a x86 processor does internally to fix up an unaligned access looks a lot like the code I have there anyhow - my takeaway is that if we felt this was computationally sensitive we should lobby to keep the headers aligned

Comment 28

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a592d6c0a4f
https://hg.mozilla.org/integration/mozilla-inbound/rev/d533854a7e2c
https://hg.mozilla.org/integration/mozilla-inbound/rev/8db316b82379

This is checked into FF 15 pref'd off. Enable with network.http.spdy.enabled.v3 = true to use it now.

we can set it to true by default in FF 16 where there will be a full nightly window for fixes and testing.

Comment 29

[Patrick McManus [:mcmanus]]

thanks honza - I guess now that we are flow control enabled server push is next!

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a592d6c0a4f
https://hg.mozilla.org/mozilla-central/rev/d533854a7e2c
https://hg.mozilla.org/mozilla-central/rev/8db316b82379

Comment 31

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This bug introduced a bunch of MPL 1.1 headers.

Comment 32

[Patrick McManus [:mcmanus]]

(In reply to Kyle Huey [:khuey] ([e-mail]) [On vacation until June 4th] from comment #31)
> This bug introduced a bunch of MPL 1.1 headers.

thanks - I'll update the MPL header.

That's going to be a recurring theme I think :)

Comment 33

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Indeed.

Comment 34

[Patrick McManus [:mcmanus]]

mpl fixup

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

Comment 35

[Ed Morley [:emorley]]

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

Comment 36

[Jean-Yves Perrier [:teoli]]

Removing dev-doc-needed: we will never document SPDY/3, but we will document HTTP/2.