[SPDY] Refactor SpdySession's write queue

net/spdy/spdy_write_queue.cc

Index: net/spdy/spdy_write_queue.cc
diff --git a/net/spdy/spdy_write_queue.cc b/net/spdy/spdy_write_queue.cc
new file mode 100644
index 0000000000000000000000000000000000000000..79d60cd3fa353b6ba80da279c4fb81ac9e19fafc
--- /dev/null
+++ b/net/spdy/spdy_write_queue.cc
@@ -0,0 +1,106 @@
+// Copyright (c) 2013 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.
+
+#include "net/spdy/spdy_write_queue.h"
+
+#include <cstddef>
+
+#include "base/logging.h"
+#include "net/spdy/spdy_stream.h"
+
+namespace net {
+
+SpdyFrameProducer::SpdyFrameProducer() {}
+
+SpdyFrameProducer::~SpdyFrameProducer() {}
+
+SimpleFrameProducer::SimpleFrameProducer(scoped_ptr<SpdyFrame> frame)
+    : frame_(frame.Pass()) {}
+
+SimpleFrameProducer::~SimpleFrameProducer() {}
+
+scoped_ptr<SpdyFrame> SimpleFrameProducer::ProduceFrame() {
+  DCHECK(frame_);
+  return frame_.Pass();
+}
+
+SpdyWriteQueue::PendingWrite::PendingWrite() : frame_producer(NULL) {}
+
+SpdyWriteQueue::PendingWrite::PendingWrite(
+    SpdyFrameProducer* frame_producer,
+    const scoped_refptr<SpdyStream>& stream)
+    : frame_producer(frame_producer),
+      stream(stream) {}
+
+SpdyWriteQueue::PendingWrite::~PendingWrite() {}
+
+SpdyWriteQueue::SpdyWriteQueue() {}
+
+SpdyWriteQueue::~SpdyWriteQueue() {
+  Clear();
+}
+
+void SpdyWriteQueue::Enqueue(RequestPriority priority,
+                             scoped_ptr<SpdyFrameProducer> frame_producer,
+                             const scoped_refptr<SpdyStream>& stream) {
+  if (stream.get()) {
+    DCHECK_EQ(stream->priority(), priority);

[Ryan Hamilton, 2013/03/28 15:51:04]

alternatively, you could have two Enqueue methods, one which takes a stream, and
one which takes a priority.  *shrug*  Might be cleaner, might be more code.

[akalin, 2013/04/06 01:17:48]

Eh, I think a single Enqueue is simpler. Added a comment re. stream->priority()
being required to equal priority.

+  }
+  queue_[priority].push_back(PendingWrite(frame_producer.release(), stream));
+}
+
+bool SpdyWriteQueue::Dequeue(scoped_ptr<SpdyFrameProducer>* frame_producer,
+                             scoped_refptr<SpdyStream>* stream) {
+  for (int i = NUM_PRIORITIES - 1; i >= 0; --i) {
+    if (!queue_[i].empty()) {

[Ryan Hamilton, 2013/03/28 15:51:04]

nit: I think I would write this as:
if (queue_[i].empty())
  continue;

early "return" ftw which lets you out-dent the rest of the block.

[akalin, 2013/04/06 01:17:48]

The thing is, I think that's more confusing because the loop body is itself an
early return! So I think it's clearer to keep the existing flow, even though it
is more indented, rather than have two different types of early-exits from the
loop.

+      PendingWrite pending_write = queue_[i].front();
+      queue_[i].pop_front();
+      frame_producer->reset(pending_write.frame_producer);
+      *stream = pending_write.stream;
+      return true;
+    }
+  }
+  return false;
+}
+
+void SpdyWriteQueue::RemovePendingWritesForStream(
+    const scoped_refptr<SpdyStream>& stream) {
+  DCHECK(stream.get());
+  if (DCHECK_IS_ON()) {
+    // |stream| should not have pending writes in a queue not matching
+    // its priority.
+    for (int i = 0; i < NUM_PRIORITIES; ++i) {
+      if (stream->priority() == i)
+        continue;
+      for (std::deque<PendingWrite>::const_iterator it = queue_[i].begin();
+           it != queue_[i].end(); ++it) {
+        DCHECK_NE(it->stream, stream);
+      }
+    }
+  }
+
+  std::deque<PendingWrite> old_queue;
+  old_queue.swap(queue_[stream->priority()]);
+
+  for (std::deque<PendingWrite>::const_iterator it = old_queue.begin();
+       it != old_queue.end(); ++it) {
+    if (it->stream == stream) {
+      delete it->frame_producer;
+    } else {
+      queue_[stream->priority()].push_back(*it);

[Ryan Hamilton, 2013/03/28 15:51:04]

It looks like deque support an erase() method which returns an iterator to the
next element after the erased element.  I think that might be better than
reconstructing the deque.

Out of curiosity, would an std::list work as well as a deque here?

[akalin, 2013/04/06 01:17:48]

It does have erase(), but it's O(distance from end). However, there's no reason
to make a copy -- rewrote the loop to shift elements up and then truncate.

a std::list would also work, but I don't like it because of the extra node
overhead.

[Ryan Hamilton, 2013/04/08 16:17:14]

Good to know!  I'm unfamiliar with the internal list impl.  Can you say more?

+    }
+  }
+}
+
+void SpdyWriteQueue::Clear() {
+  for (int i = 0; i < NUM_PRIORITIES; ++i) {
+    for (std::deque<PendingWrite>::iterator it = queue_[i].begin();
+         it != queue_[i].end(); ++it) {
+      delete it->frame_producer;
+    }
+    queue_[i].clear();
+  }
+}
+
+}  // namespace net