QUIC refactor CL to move some methods and give the QuicSentPacketManager

control over when the retransmission alarm is reset.

Merge internal change: 58352761

R=rch@chromium.org

Review URL: https://codereview.chromium.org/118443007

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@242808 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 44 insertions, 44 deletions

diff --git a/net/quic/quic_connection.cc b/net/quic/quic_connection.cc
index 5cc630d..03957a3 100644
--- a/net/quic/quic_connection.cc
+++ b/net/quic/quic_connection.cc
@@ -150,21 +150,6 @@ net::QuicConnection::Force HasForcedFrames(
   return net::QuicConnection::NO_FORCE;
 }
 
-net::IsHandshake HasCryptoHandshake(
-    const RetransmittableFrames* retransmittable_frames) {
-  if (!retransmittable_frames) {
-    return net::NOT_HANDSHAKE;
-  }
-  for (size_t i = 0; i < retransmittable_frames->frames().size(); ++i) {
-    if (retransmittable_frames->frames()[i].type == STREAM_FRAME &&
-        retransmittable_frames->frames()[i].stream_frame->stream_id ==
-            kCryptoStreamId) {
-      return net::IS_HANDSHAKE;
-    }
-  }
-  return net::NOT_HANDSHAKE;
-}
-
 }  // namespace
 
 #define ENDPOINT (is_server_ ? "Server: " : " Client: ")
@@ -202,7 +187,7 @@ QuicConnection::QuicConnection(QuicGuid guid,
       overall_connection_timeout_(QuicTime::Delta::Infinite()),
       creation_time_(clock_->ApproximateNow()),
       time_of_last_received_packet_(clock_->ApproximateNow()),
-      time_of_last_sent_packet_(clock_->ApproximateNow()),
+      time_of_last_sent_new_packet_(clock_->ApproximateNow()),
       sequence_number_of_last_inorder_packet_(0),
       sent_packet_manager_(is_server, this, clock_, kTCP),
       version_negotiation_state_(START_NEGOTIATION),
@@ -998,7 +983,7 @@ void QuicConnection::WritePendingRetransmissions() {
         sent_packet_manager_.NextPendingRetransmission();
     if (HasForcedFrames(&pending.retransmittable_frames) == NO_FORCE &&
         !CanWrite(pending.transmission_type, HAS_RETRANSMITTABLE_DATA,
-                  HasCryptoHandshake(&pending.retransmittable_frames))) {
+                  pending.retransmittable_frames.HasCryptoHandshake())) {
       break;
     }
 
@@ -1258,7 +1243,6 @@ bool QuicConnection::OnPacketSent(WriteResult result) {
   QuicPacketSequenceNumber sequence_number = pending_write_->sequence_number;
   TransmissionType transmission_type  = pending_write_->transmission_type;
   HasRetransmittableData retransmittable = pending_write_->retransmittable;
-  bool is_fec_packet = pending_write_->is_fec_packet;
   size_t length = pending_write_->length;
   pending_write_.reset();
 
@@ -1271,7 +1255,7 @@ bool QuicConnection::OnPacketSent(WriteResult result) {
 
   QuicTime now = clock_->Now();
   if (transmission_type == NOT_RETRANSMISSION) {
-    time_of_last_sent_packet_ = now;
+    time_of_last_sent_new_packet_ = now;
   }
   DVLOG(1) << ENDPOINT << "time of last sent packet: "
            << now.ToDebuggingValue();
@@ -1283,20 +1267,16 @@ bool QuicConnection::OnPacketSent(WriteResult result) {
       sent_packet_manager_.BandwidthEstimate().ToBytesPerPeriod(
           sent_packet_manager_.SmoothedRtt()));
 
-  sent_packet_manager_.OnPacketSent(sequence_number, now, length,
-                                    transmission_type, retransmittable);
+  bool reset_retransmission_alarm =
+      sent_packet_manager_.OnPacketSent(sequence_number, now, length,
+                                        transmission_type, retransmittable);
 
-  // Set the retransmit alarm only when we have sent the packet to the client
-  // and not when it goes to the pending queue, otherwise we will end up adding
-  // an entry to retransmission_timeout_ every time we attempt a write.
-  // Do not set the retransmission alarm if we're already handling one, since
-  // it will be reset when OnRetransmissionTimeout completes.
-  if ((retransmittable == HAS_RETRANSMITTABLE_DATA || is_fec_packet) &&
-      !retransmission_alarm_->IsSet()) {
-    QuicTime retransmission_time =
-        sent_packet_manager_.GetRetransmissionTime();
-    DCHECK(retransmission_time != QuicTime::Zero());
-    retransmission_alarm_->Set(retransmission_time);
+  if (reset_retransmission_alarm) {
+    retransmission_alarm_->Cancel();
+    QuicTime retransmission_time = sent_packet_manager_.GetRetransmissionTime();
+    if (retransmission_time != QuicTime::Zero()) {
+      retransmission_alarm_->Set(retransmission_time);
+    }
   }
 
   stats_.bytes_sent += result.bytes_written;
@@ -1332,7 +1312,8 @@ QuicPacketSequenceNumber QuicConnection::GetNextPacketSequenceNumber() {
 bool QuicConnection::SendOrQueuePacket(EncryptionLevel level,
                                        const SerializedPacket& packet,
                                        TransmissionType transmission_type) {
-  IsHandshake handshake = HasCryptoHandshake(packet.retransmittable_frames);
+  IsHandshake handshake = packet.retransmittable_frames == NULL ?
+      NOT_HANDSHAKE : packet.retransmittable_frames->HasCryptoHandshake();
   Force forced = HasForcedFrames(packet.retransmittable_frames);
   HasRetransmittableData retransmittable =
       (transmission_type != NOT_RETRANSMISSION ||
@@ -1607,7 +1588,7 @@ void QuicConnection::SetOverallConnectionTimeout(QuicTime::Delta timeout) {
 bool QuicConnection::CheckForTimeout() {
   QuicTime now = clock_->ApproximateNow();
   QuicTime time_of_last_packet = std::max(time_of_last_received_packet_,
-                                          time_of_last_sent_packet_);
+                                          time_of_last_sent_new_packet_);
 
   // |delta| can be < 0 as |now| is approximate time but |time_of_last_packet|
   // is accurate time. However, this should not change the behavior of
diff --git a/net/quic/quic_connection.h b/net/quic/quic_connection.h
index 5fefd6e..22282ac 100644
--- a/net/quic/quic_connection.h
+++ b/net/quic/quic_connection.h
@@ -685,8 +685,9 @@ class NET_EXPORT_PRIVATE QuicConnection
   // This is used for timeouts, and does not indicate the packet was processed.
   QuicTime time_of_last_received_packet_;
 
-  // The time that we last sent a packet for this connection.
-  QuicTime time_of_last_sent_packet_;
+  // The last time a new (non-retransmitted) packet was sent for this
+  // connection.
+  QuicTime time_of_last_sent_new_packet_;
 
   // Sequence number of the last packet guaranteed to be sent in packet sequence
   // number order.  Not set when packets are queued, since that may cause
diff --git a/net/quic/quic_protocol.cc b/net/quic/quic_protocol.cc
index 07a2f4d..a45c4af 100644
--- a/net/quic/quic_protocol.cc
+++ b/net/quic/quic_protocol.cc
@@ -424,6 +424,16 @@ const QuicFrame& RetransmittableFrames::AddNonStreamFrame(
   return frames_.back();
 }
 
+IsHandshake RetransmittableFrames::HasCryptoHandshake() const {
+  for (size_t i = 0; i < frames().size(); ++i) {
+    if (frames()[i].type == STREAM_FRAME &&
+        frames()[i].stream_frame->stream_id == kCryptoStreamId) {
+      return IS_HANDSHAKE;
+    }
+  }
+  return NOT_HANDSHAKE;
+}
+
 void RetransmittableFrames::set_encryption_level(EncryptionLevel level) {
   encryption_level_ = level;
 }
diff --git a/net/quic/quic_protocol.h b/net/quic/quic_protocol.h
index d4bdd07..db49a7c 100644
--- a/net/quic/quic_protocol.h
+++ b/net/quic/quic_protocol.h
@@ -887,6 +887,8 @@ class NET_EXPORT_PRIVATE RetransmittableFrames {
   const QuicFrame& AddNonStreamFrame(const QuicFrame& frame);
   const QuicFrames& frames() const { return frames_; }
 
+  IsHandshake HasCryptoHandshake() const;
+
   void set_encryption_level(EncryptionLevel level);
   EncryptionLevel encryption_level() const {
     return encryption_level_;
diff --git a/net/quic/quic_sent_packet_manager.cc b/net/quic/quic_sent_packet_manager.cc
index bb7f496..7657eff 100644
--- a/net/quic/quic_sent_packet_manager.cc
+++ b/net/quic/quic_sent_packet_manager.cc
@@ -464,7 +464,7 @@ SequenceNumberSet QuicSentPacketManager::GetUnackedPackets() const {
   return unacked_packets;
 }
 
-void QuicSentPacketManager::OnPacketSent(
+bool QuicSentPacketManager::OnPacketSent(
     QuicPacketSequenceNumber sequence_number,
     QuicTime sent_time,
     QuicByteCount bytes,
@@ -480,14 +480,19 @@ void QuicSentPacketManager::OnPacketSent(
                                      has_retransmittable_data)) {
     DCHECK(unacked_packets_[sequence_number].retransmittable_frames == NULL);
     unacked_packets_.erase(sequence_number);
-    return;
+    // Do not reset the retransmission timer, since the packet isn't tracked.
+    return false;
   }
 
+  // Set the retransmission timer for the first pending packet.
+  const bool set_retransmission_timer = pending_packets_.empty();
   unacked_packets_[sequence_number].sent_time = sent_time;
   packet_history_map_[sequence_number] =
       new SendAlgorithmInterface::SentPacket(bytes, sent_time);
   pending_packets_.insert(sequence_number);
   CleanupPacketHistory();
+
+  return set_retransmission_timer;
 }
 
 void QuicSentPacketManager::OnRetransmissionTimeout() {
@@ -662,7 +667,7 @@ QuicTime::Delta QuicSentPacketManager::TimeUntilSend(
 // any benefits, but if the delayed ack becomes a significant source
 // of (likely, tail) latency, then consider such a mechanism.
 
-const QuicTime::Delta QuicSentPacketManager::DelayedAckTime() {
+const QuicTime::Delta QuicSentPacketManager::DelayedAckTime() const {
   return QuicTime::Delta::FromMilliseconds(kMinRetransmissionTimeMs/2);
 }
 
diff --git a/net/quic/quic_sent_packet_manager.h b/net/quic/quic_sent_packet_manager.h
index 70129c8..85554b4 100644
--- a/net/quic/quic_sent_packet_manager.h
+++ b/net/quic/quic_sent_packet_manager.h
@@ -140,8 +140,9 @@ class NET_EXPORT_PRIVATE QuicSentPacketManager {
       const QuicTime& feedback_receive_time);
 
   // Called when we have sent bytes to the peer.  This informs the manager both
-  // the number of bytes sent and if they were retransmitted.
-  virtual void OnPacketSent(QuicPacketSequenceNumber sequence_number,
+  // the number of bytes sent and if they were retransmitted.  Returns true if
+  // the sender should reset the retransmission timer.
+  virtual bool OnPacketSent(QuicPacketSequenceNumber sequence_number,
                             QuicTime sent_time,
                             QuicByteCount bytes,
                             TransmissionType transmission_type,
@@ -165,11 +166,11 @@ class NET_EXPORT_PRIVATE QuicSentPacketManager {
                                         IsHandshake handshake);
 
   // Returns amount of time for delayed ack timer.
-  const QuicTime::Delta DelayedAckTime();
+  const QuicTime::Delta DelayedAckTime() const;
 
   // Returns the current delay for the retransmission timer, which may send
-  // either a tail loss probe or do a full RTO.  Retrans QuicTime::Zero() if
-  // there are no outstanding packets to abandon or retransmit.
+  // either a tail loss probe or do a full RTO.  Returns QuicTime::Zero() if
+  // there are no retransmittable packets.
   const QuicTime GetRetransmissionTime() const;
 
   // Returns the current RTO delay.