[SPDY] Refactor SpdySession's write queue

net/spdy/spdy_session.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef NET_SPDY_SPDY_SESSION_H_
 #define NET_SPDY_SPDY_SESSION_H_
 
 #include <deque>
-#include <list>
 #include <map>
-#include <queue>
 #include <set>
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/time.h"
 #include "net/base/io_buffer.h"
 #include "net/base/load_states.h"
 #include "net/base/net_errors.h"
+#include "net/base/net_export.h"
 #include "net/base/request_priority.h"
 #include "net/socket/client_socket_handle.h"
 #include "net/socket/ssl_client_socket.h"
 #include "net/socket/stream_socket.h"
 #include "net/spdy/buffered_spdy_framer.h"
 #include "net/spdy/spdy_credential_state.h"
 #include "net/spdy/spdy_header_block.h"
 #include "net/spdy/spdy_io_buffer.h"
 #include "net/spdy/spdy_protocol.h"
 #include "net/spdy/spdy_session_pool.h"
@@ skipping 124 matching lines @@
   scoped_refptr<SpdySession> session_;
   scoped_refptr<SpdyStream> stream_;
   GURL url_;
   RequestPriority priority_;
   BoundNetLog net_log_;
   CompletionCallback callback_;
 
   DISALLOW_COPY_AND_ASSIGN(SpdyStreamRequest);
 };
 
+// The interface for something which provides a SpdyFrame for writing.
+class NET_EXPORT_PRIVATE SpdyFrameProducer {
+public:
+  // Returns the SpdyStream associated with the frame to be produced,
+  // or NULL if the frame is associated with the session itself.
+  virtual SpdyStream* GetStream() = 0;

[Ryan Hamilton, 2013/03/24 15:22:54]

See this comment on the CL which landed SpdyIOBufferProducer:

https://chromiumcodereview.appspot.com/10448083/diff/22001/net/spdy/spdy_sess...

I had such a method in my initial implementation, but willchan requested it be
removed.  Can you refactor accordingly?

[akalin, 2013/03/24 18:24:14]

Hmm I looked at the comment but willchan didn't state his reason.

I think it's simpler to have this method, as it avoids the need for the extra
map and lets us move the activate-if-necessary logic back to the SpdySession.

willchan, can you elaborate on why you don't like this method?

[willchan no longer on Chromium, 2013/03/24 18:58:22]

Layering. This is at the frame level, which is lower level than streams. It
feels like a minor layering violation (kind of like RTTI), especially since not
all frames have streams. A HasStream() or GetStreamId() wouldn be less of a
violation that exposing a raw SpdyStream*.

Now, layers are obviously arbitrary. Oftentimes tighter coupling and removal of
abstraction layers can simplify code. There are tradeoffs here. I generally err
on the side of maintaining abstraction boundaries throughout most of my code
unless it's obviously too cumbersome and would be much simpler to eliminate the
arbitrary mental layer.

I leave that up to you guys. 

+
+  // Produces the frame to be written. Will be called at most once.
+  virtual scoped_ptr<SpdyFrame> ProduceFrame() = 0;
+
+  virtual ~SpdyFrameProducer() {}
+};
+
 class NET_EXPORT SpdySession : public base::RefCounted<SpdySession>,
                                public BufferedSpdyFramerVisitorInterface {
  public:
   // TODO(akalin): Use base::TickClock when it becomes available.
   typedef base::TimeTicks (*TimeFunc)(void);
 
   // How we handle flow control (version-dependent).
   enum FlowControlState {
     FLOW_CONTROL_NONE,
     FLOW_CONTROL_STREAM,
     FLOW_CONTROL_STREAM_AND_SESSION
   };
 
-  // Defines an interface for producing SpdyIOBuffers.
-  class NET_EXPORT_PRIVATE SpdyIOBufferProducer {
-   public:
-    SpdyIOBufferProducer() {}
-
-    // Returns a newly created SpdyIOBuffer, owned by the caller, or NULL
-    // if not buffer is ready to be produced.
-    virtual SpdyIOBuffer* ProduceNextBuffer(SpdySession* session) = 0;
-
-    virtual RequestPriority GetPriority() const = 0;
-
-    virtual ~SpdyIOBufferProducer() {}
-
-   protected:
-    // Activates |spdy_stream| in |spdy_session|.
-    static void ActivateStream(SpdySession* spdy_session,
-                               SpdyStream* spdy_stream);
-
-    static SpdyIOBuffer* CreateIOBuffer(SpdyFrame* frame,
-                                        RequestPriority priority,
-                                        SpdyStream* spdy_stream);
-  };
-
   // Create a new SpdySession.
   // |host_port_proxy_pair| is the host/port that this session connects to, and
   // the proxy configuration settings that it's using.
   // |spdy_session_pool| is the SpdySessionPool that owns us.  Its lifetime must
   // strictly be greater than |this|.
   // |session| is the HttpNetworkSession.  |net_log| is the NetLog that we log
   // network events to.
   SpdySession(const HostPortProxyPair& host_port_proxy_pair,
               SpdySessionPool* spdy_session_pool,
               HttpServerProperties* http_server_properties,
@@ skipping 43 matching lines @@
   // this session provides authentication for the domain and no client
   // certificate or channel ID was sent to the original server during the SSL
   // handshake.  NOTE:  This function can have false negatives on some
   // platforms.
   // TODO(wtc): rename this function and the Net.SpdyIPPoolDomainMatch
   // histogram because this function does more than verifying domain
   // authentication now.
   bool VerifyDomainAuthentication(const std::string& domain);
 
   // Records that |stream| has a write available from |producer|.
-  // |producer| will be owned by this SpdySession.
   void SetStreamHasWriteAvailable(SpdyStream* stream,
-                                  SpdyIOBufferProducer* producer);
+                                  scoped_ptr<SpdyFrameProducer> producer);
 
   // Send the SYN frame for |stream_id|. This also sends PING message to check
   // the status of the connection.
   SpdyFrame* CreateSynStream(
       SpdyStreamId stream_id,
       RequestPriority priority,
       uint8 credential_slot,
       SpdyControlFlags flags,
       const SpdyHeaderBlock& headers);
 
@@ skipping 189 matching lines @@
   FRIEND_TEST_ALL_PREFIXES(SpdySessionSpdy3Test, SessionFlowControlEndToEnd31);
 
   typedef std::deque<SpdyStreamRequest*> PendingStreamRequestQueue;
   typedef std::set<SpdyStreamRequest*> PendingStreamRequestCompletionSet;
 
   typedef std::map<int, scoped_refptr<SpdyStream> > ActiveStreamMap;
   typedef std::map<std::string,
       std::pair<scoped_refptr<SpdyStream>, base::TimeTicks> > PushedStreamMap;
 
   typedef std::set<scoped_refptr<SpdyStream> > CreatedStreamSet;
-  typedef std::map<SpdyIOBufferProducer*, SpdyStream*> StreamProducerMap;
-
-  class SpdyIOBufferProducerCompare {
-   public:
-    bool operator() (const SpdyIOBufferProducer* lhs,
-                     const SpdyIOBufferProducer* rhs) const {
-      return lhs->GetPriority() < rhs->GetPriority();
-    }
-  };
-
-  typedef std::priority_queue<SpdyIOBufferProducer*,
-                              std::vector<SpdyIOBufferProducer*>,
-                              SpdyIOBufferProducerCompare> WriteQueue;
 
   enum State {
     STATE_IDLE,
     STATE_CONNECTING,
     STATE_DO_READ,
     STATE_DO_READ_COMPLETE,
     STATE_CLOSED
   };
 
   virtual ~SpdySession();
@@ skipping 74 matching lines @@
   // haven't received any data in |kHungInterval| time period.
   void CheckPingStatus(base::TimeTicks last_check_time);
 
   // Write current data to the socket.
   void WriteSocketLater();
   void WriteSocket();
 
   // Get a new stream id.
   int GetNewStreamId();
 
-  // Queue a frame for sending.
+  // Delegates to QueueFrameProducerForWriting to put the given frame
+  // associated with the session on the write queue.
+  //
   // |frame| is the frame to send.
   // |priority| is the priority for insertion into the queue.
-  void QueueFrame(SpdyFrame* frame, RequestPriority priority);
+  void QueueSessionFrameForWriting(scoped_ptr<SpdyFrame> frame,
+                                   RequestPriority priority);
+
+  // Puts |producer| onto the write queue with the given priority.
+  void QueueFrameProducerForWriting(scoped_ptr<SpdyFrameProducer> producer,
+                                    RequestPriority priority);
+
+  // Pops the next frame producer to send from the write queue, or
+  // NULL if there isn't one.
+  scoped_ptr<SpdyFrameProducer> PopNextFrameProducerToWrite();
+
+  // Remove all frame producers associated with the given stream from
+  // the write queue.
+  void RemoveStreamFromWriteQueue(const scoped_refptr<SpdyStream>& stream);
+
+  // Remove everything from the write queue.
+  void ClearWriteQueue();
 
   // Track active streams in the active stream list.
   void ActivateStream(SpdyStream* stream);
   void DeleteStream(SpdyStreamId id, int status);
 
   // Removes this session from the session pool.
   void RemoveFromPool();
 
   // Check if we have a pending pushed-stream for this url
   // Returns the stream if found (and returns it from the pending
@@ skipping 162 matching lines @@
   // them?
   ActiveStreamMap active_streams_;
 
   // Map of all the streams that have already started to be pushed by the
   // server, but do not have consumers yet.
   PushedStreamMap unclaimed_pushed_streams_;
 
   // Set of all created streams but that have not yet sent any frames.
   CreatedStreamSet created_streams_;
 
-  // As streams have data to be sent, we put them into the write queue.
-  WriteQueue write_queue_;
-
-  // Mapping from SpdyIOBufferProducers to their corresponding SpdyStream
-  // so that when a stream is destroyed, we can remove the corresponding
-  // producer from |write_queue_|.
-  StreamProducerMap stream_producers_;
+  // A FIFO queue of data to be sent, binned by priority.
+  std::deque<SpdyFrameProducer*> write_queue_[NUM_PRIORITIES];
 
   // The packet we are currently sending.
   bool write_pending_;            // Will be true when a write is in progress.
   SpdyIOBuffer in_flight_write_;  // This is the write buffer in progress.
 
   // Flag if we have a pending message scheduled for WriteSocket.
   bool delayed_write_pending_;
 
   // Flag if we're using an SSL connection for this SpdySession.
   bool is_secure_;
@@ skipping 115 matching lines @@
   // This SPDY proxy is allowed to push resources from origins that are
   // different from those of their associated streams.
   HostPortPair trusted_spdy_proxy_;
 
   TimeFunc time_func_;
 };
 
 }  // namespace net
 
 #endif  // NET_SPDY_SPDY_SESSION_H_