bug 708415 spdysession review comments r=honzab

netwerk/protocol/http/SpdySession.h

@@ -132,11 +132,15 @@ public:
   // but if it needs to grow for huge headers it can do so dynamically.
   // About 1% of requests to SPDY google services seem to be > 1000
   // with all less than 2000.
-  const static PRUint32 kDefaultBufferSize = 2000;
+  const static PRUint32 kDefaultBufferSize = 2048;
 
-  const static PRUint32 kDefaultQueueSize =  16000;
-  const static PRUint32 kQueueTailRoom    =  4000;
-  const static PRUint32 kSendingChunkSize = 4000;
+  // kDefaultQueueSize must be >= other queue size constants
+  const static PRUint32 kDefaultQueueSize =  16384;
+  const static PRUint32 kQueueMinimumCleanup = 8192;
+  const static PRUint32 kQueueTailRoom    =  4096;
+  const static PRUint32 kQueueReserved    =  1024;
+
+  const static PRUint32 kSendingChunkSize = 4096;
   const static PRUint32 kDefaultMaxConcurrent = 100;
   const static PRUint32 kMaxStreamID = 0x7800000;
   
@@ -157,19 +161,23 @@ public:
   static void LogIO(SpdySession *, SpdyStream *, const char *,
                     const char *, PRUint32);
 
+  // an overload of nsAHttpConnection
+  void TransactionHasDataToWrite(nsAHttpTransaction *);
+
 private:
 
   enum stateType {
     BUFFERING_FRAME_HEADER,
     BUFFERING_CONTROL_FRAME,
     PROCESSING_DATA_FRAME,
-    DISCARD_DATA_FRAME,
+    DISCARDING_DATA_FRAME,
     PROCESSING_CONTROL_SYN_REPLY,
     PROCESSING_CONTROL_RST_STREAM
   };
 
-  PRUint32    WriteQueueSize();
+  PRUint32    GetWriteQueueSize();
   void        ChangeDownstreamState(enum stateType);
+  void        ResetDownstreamState();
   nsresult    DownstreamUncompress(char *, PRUint32);
   void        zlibInit();
   nsresult    FindHeader(nsCString, nsDependentCSubstring &);
@@ -180,16 +188,16 @@ private:
   void        GenerateGoAway();
   void        CleanupStream(SpdyStream *, nsresult);
 
-  void        SetWriteCallbacks(nsAHttpTransaction *);
+  void        SetWriteCallbacks();
   void        FlushOutputQueue();
 
   bool        RoomForMoreConcurrent();
   void        ActivateStream(SpdyStream *);
   void        ProcessPending();
 
-  static PLDHashOperator Shutdown(nsAHttpTransaction *,
-                                  nsAutoPtr<SpdyStream> &,
-                                  void *);
+  static PLDHashOperator ShutdownEnumerator(nsAHttpTransaction *,
+                                            nsAutoPtr<SpdyStream> &,
+                                            void *);
 
   // This is intended to be nsHttpConnectionMgr:nsHttpConnectionHandle taken
   // from the first transaction on this session. That object contains the
@@ -230,10 +238,10 @@ private:
   // this queue in place to ease that transition.
   nsDeque           mUrgentForWrite;
 
-  // If we block while wrting out a frame then this points to the stream
+  // If we block while writing out a frame then this points to the stream
   // that was blocked. When writing again that stream must be the first
   // one to write. It is null if there is not a partial frame.
-  SpdyStream        *mPartialFrame;
+  SpdyStream        *mPartialFrameSender;
 
   // Compression contexts for header transport using deflate.
   // SPDY compresses only HTTP headers and does not reset zlib in between
@@ -241,25 +249,29 @@ private:
   z_stream            mDownstreamZlib;
   z_stream            mUpstreamZlib;
 
-  // mFrameBuffer is used to store received control packets and the 8 bytes
+  // mInputFrameBuffer is used to store received control packets and the 8 bytes
   // of header on data packets
-  PRUint32             mFrameBufferSize;
-  PRUint32             mFrameBufferUsed;
-  nsAutoArrayPtr<char> mFrameBuffer;
+  PRUint32             mInputFrameBufferSize;
+  PRUint32             mInputFrameBufferUsed;
+  nsAutoArrayPtr<char> mInputFrameBuffer;
   
-  // mFrameDataSize/Read are used for tracking the amount of data consumed
+  // mInputFrameDataSize/Read are used for tracking the amount of data consumed
   // in a data frame. the data itself is not buffered in spdy
-  // The frame size is mFrameDataSize + the constant 8 byte header
-  PRUint32             mFrameDataSize;
-  PRUint32             mFrameDataRead;
-  bool                 mFrameDataLast; // This frame was marked FIN
+  // The frame size is mInputFrameDataSize + the constant 8 byte header
+  PRUint32             mInputFrameDataSize;
+  PRUint32             mInputFrameDataRead;
+  bool                 mInputFrameDataLast; // This frame was marked FIN
 
   // When a frame has been received that is addressed to a particular stream
   // (e.g. a data frame after the stream-id has been decoded), this points
   // to the stream.
-  SpdyStream          *mFrameDataStream;
+  SpdyStream          *mInputFrameDataStream;
   
-  // A state variable to cleanup a closed stream after the stack has unwound.
+  // mNeedsCleanup is a state variable to defer cleanup of a closed stream
+  // If needed, It is set in session::OnWriteSegments() and acted on and
+  // cleared when the stack returns to session::WriteSegments(). The stream
+  // cannot be destroyed directly out of OnWriteSegments because
+  // stream::writeSegments() is on the stack at that time.
   SpdyStream          *mNeedsCleanup;
 
   // The CONTROL_TYPE value for a control frame
@@ -280,7 +292,11 @@ private:
   nsCString            mFlatHTTPResponseHeaders;
   PRUint32             mFlatHTTPResponseHeadersOut;
 
-  // when set, the session will go away when it reaches 0 streams
+  // when set, the session will go away when it reaches 0 streams. This flag
+  // is set when: the stream IDs are running out (at either the client or the
+  // server), when DontReuse() is called, a RST that is not specific to a
+  // particular stream is received, a GOAWAY frame has been received from
+  // the server.
   bool                 mShouldGoAway;
 
   // the session has received a nsAHttpTransaction::Close()  call

netwerk/protocol/http/SpdyStream.h

@@ -54,7 +54,6 @@ public:
   SpdyStream(nsAHttpTransaction *,
              SpdySession *, nsISocketTransport *,
              PRUint32, z_stream *, PRInt32);
-  ~SpdyStream();
 
   PRUint32 StreamID() { return mStreamID; }
 
@@ -100,6 +99,12 @@ public:
 
 private:
 
+  // a SpdyStream object is only destroyed by being removed from the
+  // SpdySession mStreamTransactionHash - make the dtor private to
+  // just the AutoPtr implementation needed for that hash.
+  friend class nsAutoPtr<SpdyStream>;
+  ~SpdyStream();
+
   enum stateType {
     GENERATING_SYN_STREAM,
     SENDING_SYN_STREAM,
@@ -130,7 +135,10 @@ private:
   // sending_request_body for each SPDY chunk in the upload.
   enum stateType mUpstreamState;
 
-  // The underlying HTTP transaction
+  // The underlying HTTP transaction. This pointer is used as the key
+  // in the SpdySession mStreamTransactionHash so it is important to
+  // keep a reference to it as long as this stream is a member of that hash.
+  // (i.e. don't change it or release it after it is set in the ctor).
   nsRefPtr<nsAHttpTransaction> mTransaction;
 
   // The session that this stream is a subset of

netwerk/protocol/http/nsAHttpConnection.h

@@ -77,9 +77,20 @@ public:
     // after a transaction returned NS_BASE_STREAM_WOULD_BLOCK from its
     // ReadSegments/WriteSegments methods.
     //
-    virtual nsresult ResumeSend(nsAHttpTransaction *caller) = 0;
-    virtual nsresult ResumeRecv(nsAHttpTransaction *caller) = 0;
+    virtual nsresult ResumeSend() = 0;
+    virtual nsresult ResumeRecv() = 0;
 
+    // After a connection has had ResumeSend() called by a transaction,
+    // and it is ready to write to the network it may need to know the
+    // transaction that has data to write. This is only an issue for
+    // multiplexed protocols like SPDY - plain HTTP or pipelined HTTP
+    // implicitly have this information in a 1:1 relationship with the
+    // transaction(s) they manage.
+    virtual void TransactionHasDataToWrite(nsAHttpTransaction *)
+    {
+        // by default do nothing - only multiplexed protocols need to overload
+        return;
+    }
     //
     // called by the connection manager to close a transaction being processed
     // by this connection.
@@ -132,8 +143,8 @@ public:
 
 #define NS_DECL_NSAHTTPCONNECTION \
     nsresult OnHeadersAvailable(nsAHttpTransaction *, nsHttpRequestHead *, nsHttpResponseHead *, bool *reset); \
-    nsresult ResumeSend(nsAHttpTransaction *); \
-    nsresult ResumeRecv(nsAHttpTransaction *); \
+    nsresult ResumeSend(); \
+    nsresult ResumeRecv(); \
     void CloseTransaction(nsAHttpTransaction *, nsresult); \
     void GetConnectionInfo(nsHttpConnectionInfo **); \
     nsresult TakeTransport(nsISocketTransport **,    \

netwerk/protocol/http/nsHttpConnection.cpp

@@ -390,7 +390,7 @@ nsHttpConnection::AddTransaction(nsAHttpTransaction *httpTransaction,
         return NS_ERROR_FAILURE;
     }
 
-    ResumeSend(httpTransaction);
+    ResumeSend();
 
     return NS_OK;
 }
@@ -796,7 +796,7 @@ nsHttpConnection::PushBack(const char *data, PRUint32 length)
 }
 
 nsresult
-nsHttpConnection::ResumeSend(nsAHttpTransaction *)
+nsHttpConnection::ResumeSend()
 {
     LOG(("nsHttpConnection::ResumeSend [this=%p]\n", this));
 
@@ -810,7 +810,7 @@ nsHttpConnection::ResumeSend(nsAHttpTransaction *)
 }
 
 nsresult
-nsHttpConnection::ResumeRecv(nsAHttpTransaction *)
+nsHttpConnection::ResumeRecv()
 {
     LOG(("nsHttpConnection::ResumeRecv [this=%p]\n", this));
 

netwerk/protocol/http/nsHttpConnection.h

@@ -142,8 +142,8 @@ public:
     void     SetIsReusedAfter(PRUint32 afterMilliseconds);
     void     SetIdleTimeout(PRUint16 val) {mIdleTimeout = val;}
     nsresult PushBack(const char *data, PRUint32 length);
-    nsresult ResumeSend(nsAHttpTransaction *caller);
-    nsresult ResumeRecv(nsAHttpTransaction *caller);
+    nsresult ResumeSend();
+    nsresult ResumeRecv();
     PRInt64  MaxBytesRead() {return mMaxBytesRead;}
 
     static NS_METHOD ReadFromStream(nsIInputStream *, void *, const char *,

netwerk/protocol/http/nsHttpConnectionMgr.cpp

@@ -1718,15 +1718,15 @@ nsHttpConnectionMgr::nsConnectionHandle::OnHeadersAvailable(nsAHttpTransaction *
 }
 
 nsresult
-nsHttpConnectionMgr::nsConnectionHandle::ResumeSend(nsAHttpTransaction *caller)
+nsHttpConnectionMgr::nsConnectionHandle::ResumeSend()
 {
-    return mConn->ResumeSend(caller);
+    return mConn->ResumeSend();
 }
 
 nsresult
-nsHttpConnectionMgr::nsConnectionHandle::ResumeRecv(nsAHttpTransaction *caller)
+nsHttpConnectionMgr::nsConnectionHandle::ResumeRecv()
 {
-    return mConn->ResumeRecv(caller);
+    return mConn->ResumeRecv();
 }
 
 void

netwerk/protocol/http/nsHttpHandler.cpp

@@ -77,6 +77,7 @@
 #include "nsAsyncRedirectVerifyHelper.h"
 #include "nsSocketTransportService2.h"
 #include "nsAlgorithm.h"
+#include "SpdySession.h"
 
 #include "nsIXULAppInfo.h"
 
@@ -202,6 +203,7 @@ nsHttpHandler::nsHttpHandler()
     , mEnableSpdy(false)
     , mCoalesceSpdy(true)
     , mUseAlternateProtocol(false)
+    , mSpdySendingChunkSize(SpdySession::kSendingChunkSize)
 {
 #if defined(PR_LOGGING)
     gHttpLog = PR_NewLogModule("nsHttp");
@@ -1094,6 +1096,12 @@ nsHttpHandler::PrefsChanged(nsIPrefBranch *prefs, const char *pref)
             mSpdyTimeout = (PRUint16) clamped(val, 1, 0xffff);
     }
 
+    if (PREF_CHANGED(HTTP_PREF("spdy.chunk-size"))) {
+        rv = prefs->GetIntPref(HTTP_PREF("spdy.chunk-size"), &val);
+        if (NS_SUCCEEDED(rv))
+            mSpdySendingChunkSize = (PRUint32) clamped(val, 1, 0x7fffffff);
+    }
+
     //
     // INTL options
     //

netwerk/protocol/http/nsHttpHandler.h

@@ -116,6 +116,7 @@ public:
     bool           IsSpdyEnabled() { return mEnableSpdy; }
     bool           CoalesceSpdy() { return mCoalesceSpdy; }
     bool           UseAlternateProtocol() { return mUseAlternateProtocol; }
+    PRUint32       SpdySendingChunkSize() { return mSpdySendingChunkSize; }
 
     bool           PromptTempRedirect()      { return mPromptTempRedirect; }
 
@@ -342,6 +343,7 @@ private:
     bool           mEnableSpdy;
     bool           mCoalesceSpdy;
     bool           mUseAlternateProtocol;
+    PRUint32       mSpdySendingChunkSize;
 };
 
 //-----------------------------------------------------------------------------

netwerk/protocol/http/nsHttpPipeline.cpp

@@ -131,7 +131,7 @@ nsHttpPipeline::AddTransaction(nsAHttpTransaction *trans)
         trans->SetConnection(this);
 
         if (mRequestQ.Length() == 1)
-            mConnection->ResumeSend(trans);
+            mConnection->ResumeSend();
     }
 
     return NS_OK;
@@ -170,19 +170,19 @@ nsHttpPipeline::OnHeadersAvailable(nsAHttpTransaction *trans,
 }
 
 nsresult
-nsHttpPipeline::ResumeSend(nsAHttpTransaction *trans)
+nsHttpPipeline::ResumeSend()
 {
     if (mConnection)
-        return mConnection->ResumeSend(trans);
+        return mConnection->ResumeSend();
     return NS_ERROR_UNEXPECTED;
 }
 
 nsresult
-nsHttpPipeline::ResumeRecv(nsAHttpTransaction *trans)
+nsHttpPipeline::ResumeRecv()
 {
     NS_ASSERTION(PR_GetCurrentThread() == gSocketThread, "wrong thread");
     NS_ASSERTION(mConnection, "no connection");
-    return mConnection->ResumeRecv(trans);
+    return mConnection->ResumeRecv();
 }
 
 void

netwerk/protocol/http/nsHttpTransaction.cpp

@@ -1298,7 +1298,8 @@ NS_IMETHODIMP
 nsHttpTransaction::OnInputStreamReady(nsIAsyncInputStream *out)
 {
     if (mConnection) {
-        nsresult rv = mConnection->ResumeSend(this);
+        mConnection->TransactionHasDataToWrite(this);
+        nsresult rv = mConnection->ResumeSend();
         if (NS_FAILED(rv))
             NS_ERROR("ResumeSend failed");
     }
@@ -1314,7 +1315,7 @@ NS_IMETHODIMP
 nsHttpTransaction::OnOutputStreamReady(nsIAsyncOutputStream *out)
 {
     if (mConnection) {
-        nsresult rv = mConnection->ResumeRecv(this);
+        nsresult rv = mConnection->ResumeRecv();
         if (NS_FAILED(rv))
             NS_ERROR("ResumeRecv failed");
     }