Land Recent QUIC Changes.

net/quic/quic_sent_packet_manager.cc

 // Copyright 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/quic/quic_sent_packet_manager.h"
 
 #include <algorithm>
 
 #include "base/logging.h"
 #include "base/stl_util.h"
@@ skipping 226 matching lines @@
       continue;
     }
     // If it had no other transmissions, we handle it above.  If it has
     // other transmissions, one of them must have retransmittable frames,
     // so that gets resolved the same way as other retransmissions.
     // TODO(ianswett): Consider adding a new retransmission type which removes
     // all these old packets from unacked and retransmits them as new sequence
     // numbers with no connection to the previous ones.
     if (frames != NULL && (retransmission_type == ALL_PACKETS ||
                            frames->encryption_level() == ENCRYPTION_INITIAL)) {
-      unacked_packets_.SetNotPending(unacked_it->first);
       MarkForRetransmission(unacked_it->first, ALL_UNACKED_RETRANSMISSION);
     }
     ++unacked_it;
   }
 }
 
-void QuicSentPacketManager::DiscardUnencryptedPackets() {
+void QuicSentPacketManager::NeuterUnencryptedPackets() {
   QuicUnackedPacketMap::const_iterator unacked_it = unacked_packets_.begin();
   while (unacked_it != unacked_packets_.end()) {

[wtc, 2014/05/13 18:49:23]

Nit: in the new code, this loop can be a for loop because it updates
|unacked_it| in only one way. (The old code updated |unacked_it| in two ways, so
it had to be a while loop.)

[ramant (doing other things), 2014/05/14 05:33:29]

Made this change in
https://codereview.chromium.org/286693003/diff/1/net/quic/quic_sent_packet_ma...

Done.

     const RetransmittableFrames* frames =
         unacked_it->second.retransmittable_frames;
     if (frames != NULL && frames->encryption_level() == ENCRYPTION_NONE) {
-      // Once you're forward secure, no unencrypted packets will be sent.
-      // Additionally, it's likely the peer will be forward secure, and no acks
-      // for these packets will be received, so mark the packet as handled.
+      // Since once you're forward secure, no unencrypted packets will be sent,

[wtc, 2014/05/13 18:49:23]

Nit: "Since" and "once" sound a little redundant. It seems that "once" is enough
(as in the original comment).

[ramant (doing other things), 2014/05/14 05:33:29]

Made this change in
https://codereview.chromium.org/286693003/diff/1/net/quic/quic_sent_packet_ma...

Done.

+      // crypto or otherwise. Unencrypted packets are neutered and abandoned, to
+      // ensure they are not retransmitted or considered lost from a congestion
+      // control perspective.
       pending_retransmissions_.erase(unacked_it->first);
-      unacked_it = MarkPacketHandled(unacked_it->first,
-                                     QuicTime::Delta::Zero());
-      continue;
+      // TODO(ianswett): This may cause packets to linger forever in the
+      // UnackedPacketMap.
+      unacked_packets_.NeuterPacket(unacked_it->first);
+      unacked_packets_.SetNotPending(unacked_it->first);
     }
     ++unacked_it;
   }
 }
 
 void QuicSentPacketManager::MarkForRetransmission(
     QuicPacketSequenceNumber sequence_number,
     TransmissionType transmission_type) {
   const TransmissionInfo& transmission_info =
       unacked_packets_.GetTransmissionInfo(sequence_number);
   LOG_IF(DFATAL, transmission_info.retransmittable_frames == NULL);
+  if (transmission_type != TLP_RETRANSMISSION) {
+    unacked_packets_.SetNotPending(sequence_number);
+  }
   // TODO(ianswett): Currently the RTO can fire while there are pending NACK
   // retransmissions for the same data, which is not ideal.
   if (ContainsKey(pending_retransmissions_, sequence_number)) {
     return;
   }
 
   pending_retransmissions_[sequence_number] = transmission_type;
 }
 
 bool QuicSentPacketManager::HasPendingRetransmissions() const {
@@ skipping 42 matching lines @@
 
   const TransmissionInfo& transmission_info =
       unacked_packets_.GetTransmissionInfo(sequence_number);
   // The AckNotifierManager needs to be notified for revived packets,
   // since it indicates the packet arrived from the appliction's perspective.
   if (transmission_info.retransmittable_frames) {
     ack_notifier_manager_.OnPacketAcked(
         sequence_number, delta_largest_observed);
   }
 
-  unacked_packets_.NeuterIfPendingOrRemovePacket(sequence_number);
+  if (!transmission_info.pending) {
+    unacked_packets_.RemovePacket(sequence_number);
+  } else {
+    unacked_packets_.NeuterPacket(sequence_number);
+  }
 }
 
 QuicUnackedPacketMap::const_iterator QuicSentPacketManager::MarkPacketHandled(
     QuicPacketSequenceNumber sequence_number,
     QuicTime::Delta delta_largest_observed) {
   if (!unacked_packets_.IsUnacked(sequence_number)) {
     LOG(DFATAL) << "Packet is not unacked: " << sequence_number;
     return unacked_packets_.end();
   }
   const TransmissionInfo& transmission_info =
       unacked_packets_.GetTransmissionInfo(sequence_number);
-  // If this packet is pending, remove it and inform the send algorithm.
+  // If this packet is pending, remove it.
   if (transmission_info.pending) {
     unacked_packets_.SetNotPending(sequence_number);
   }
 
   SequenceNumberSet all_transmissions = *transmission_info.all_transmissions;
   SequenceNumberSet::reverse_iterator all_transmissions_it =
       all_transmissions.rbegin();
   QuicPacketSequenceNumber newest_transmission = *all_transmissions_it;
+  // Remove the most recent packet, if it is pending retransmission.
+  pending_retransmissions_.erase(newest_transmission);
+
+  // Two cases for MarkPacketHandled:
+  // 1) Handle the most recent or a crypto packet, so remove all transmissions.
+  // 2) Handle old transmission, keep all other pending transmissions,
+  //    but disassociate them from one another.
   if (newest_transmission != sequence_number) {
     stats_->bytes_spuriously_retransmitted += transmission_info.bytes_sent;
     ++stats_->packets_spuriously_retransmitted;
   }
 
   // The AckNotifierManager needs to be notified about the most recent
   // transmission, since that's the one only one it tracks.
   ack_notifier_manager_.OnPacketAcked(newest_transmission,
                                       delta_largest_observed);
 
+  // TODO(ianswett): Instead of handling all crypto packets in a special way,
+  // only handle NULL encrypted packets in a special way.
   bool has_crypto_handshake = HasCryptoHandshake(
       unacked_packets_.GetTransmissionInfo(newest_transmission));
   while (all_transmissions_it != all_transmissions.rend()) {
     QuicPacketSequenceNumber previous_transmission = *all_transmissions_it;
-    // If this packet was marked for retransmission, don't bother retransmitting
-    // it anymore.
-    pending_retransmissions_.erase(previous_transmission);
+    const TransmissionInfo& transmission_info =
+        unacked_packets_.GetTransmissionInfo(previous_transmission);
+    DCHECK(!ContainsKey(pending_retransmissions_, previous_transmission));
     if (has_crypto_handshake) {
       // If it's a crypto handshake packet, discard it and all retransmissions,
       // since they won't be acked now that one has been processed.
       unacked_packets_.SetNotPending(previous_transmission);
     }
-    unacked_packets_.NeuterIfPendingOrRemovePacket(previous_transmission);
+    if (!transmission_info.pending) {
+      unacked_packets_.RemovePacket(previous_transmission);
+    } else {
+      unacked_packets_.NeuterPacket(previous_transmission);
+    }
     ++all_transmissions_it;
   }
 
   QuicUnackedPacketMap::const_iterator next_unacked = unacked_packets_.begin();
   while (next_unacked != unacked_packets_.end() &&
          next_unacked->first < sequence_number) {
     ++next_unacked;
   }
   return next_unacked;
 }
@@ skipping 89 matching lines @@
        it != unacked_packets_.end(); ++it) {
     QuicPacketSequenceNumber sequence_number = it->first;
     const RetransmittableFrames* frames = it->second.retransmittable_frames;
     // Only retransmit frames which are pending, and therefore have been sent.
     if (!it->second.pending || frames == NULL ||
         frames->HasCryptoHandshake() != IS_HANDSHAKE) {
       continue;
     }
     packet_retransmitted = true;
     MarkForRetransmission(sequence_number, HANDSHAKE_RETRANSMISSION);
-    // Abandon all the crypto retransmissions now so they're not lost later.
-    unacked_packets_.SetNotPending(sequence_number);
   }
   DCHECK(packet_retransmitted) << "No crypto packets found to retransmit.";
 }
 
 void QuicSentPacketManager::RetransmitOldestPacket() {
   DCHECK_EQ(TLP_MODE, GetRetransmissionMode());
   ++consecutive_tlp_count_;
   for (QuicUnackedPacketMap::const_iterator it = unacked_packets_.begin();
        it != unacked_packets_.end(); ++it) {
     QuicPacketSequenceNumber sequence_number = it->first;
@@ skipping 17 matching lines @@
   DVLOG(1) << "OnRetransmissionTimeout() fired with "
            << unacked_packets_.GetNumUnackedPackets() << " unacked packets.";
 
   // Request retransmission of all retransmittable packets when the RTO
   // fires, and let the congestion manager decide how many to send
   // immediately and the remaining packets will be queued.
   // Abandon any non-retransmittable packets that are sufficiently old.
   bool packets_retransmitted = false;
   for (QuicUnackedPacketMap::const_iterator it = unacked_packets_.begin();
        it != unacked_packets_.end(); ++it) {
-    unacked_packets_.SetNotPending(it->first);
     if (it->second.retransmittable_frames != NULL) {
       packets_retransmitted = true;
       MarkForRetransmission(it->first, RTO_RETRANSMISSION);
+    } else {
+      unacked_packets_.SetNotPending(it->first);
     }
   }
 
   send_algorithm_->OnRetransmissionTimeout(packets_retransmitted);
   if (packets_retransmitted) {
     ++consecutive_rto_count_;
   }
 }
 
 QuicSentPacketManager::RetransmissionTimeoutMode
@@ skipping 29 matching lines @@
   for (SequenceNumberSet::const_iterator it = lost_packets.begin();
        it != lost_packets.end(); ++it) {
     QuicPacketSequenceNumber sequence_number = *it;
     const TransmissionInfo& transmission_info =
         unacked_packets_.GetTransmissionInfo(sequence_number);
     // TODO(ianswett): If it's expected the FEC packet may repair the loss, it
     // should be recorded as a loss to the send algorithm, but not retransmitted
     // until it's known whether the FEC packet arrived.
     ++stats_->packets_lost;
     packets_lost_[sequence_number] = transmission_info;
-    unacked_packets_.SetNotPending(sequence_number);
+    DVLOG(1) << ENDPOINT << "Lost packet " << sequence_number;
 
     if (transmission_info.retransmittable_frames != NULL) {
       MarkForRetransmission(sequence_number, LOSS_RETRANSMISSION);
     } else {
       // Since we will not retransmit this, we need to remove it from
       // unacked_packets_.   This is either the current transmission of
-      // a packet whose previous transmission has been acked, or it
-      // is a packet that has been TLP retransmitted.
-      unacked_packets_.NeuterIfPendingOrRemovePacket(sequence_number);
+      // a packet whose previous transmission has been acked, a packet that has
+      // been TLP retransmitted, or an FEC packet.
+      unacked_packets_.SetNotPending(sequence_number);
+      unacked_packets_.RemovePacket(sequence_number);
     }
   }
 }
 
 bool QuicSentPacketManager::MaybeUpdateRTT(
     const ReceivedPacketInfo& received_info,
     const QuicTime& ack_receive_time) {
   if (!unacked_packets_.IsUnacked(received_info.largest_observed)) {
     return false;
   }
@@ skipping 51 matching lines @@
     case HANDSHAKE_MODE:
       return clock_->ApproximateNow().Add(GetCryptoRetransmissionDelay());
     case LOSS_MODE:
       return loss_algorithm_->GetLossTimeout();
     case TLP_MODE: {
       // TODO(ianswett): When CWND is available, it would be preferable to
       // set the timer based on the earliest retransmittable packet.
       // Base the updated timer on the send time of the last packet.
       const QuicTime sent_time = unacked_packets_.GetLastPacketSentTime();
       const QuicTime tlp_time = sent_time.Add(GetTailLossProbeDelay());
-      // Ensure the tlp timer never gets set to a time in the past.
+      // Ensure the TLP timer never gets set to a time in the past.
       return QuicTime::Max(clock_->ApproximateNow(), tlp_time);
     }
     case RTO_MODE: {
       // The RTO is based on the first pending packet.
       const QuicTime sent_time =
           unacked_packets_.GetFirstPendingPacketSentTime();
       QuicTime rto_timeout = sent_time.Add(GetRetransmissionDelay());
       // Always wait at least 1.5 * RTT from now.
       QuicTime min_timeout = clock_->ApproximateNow().Add(
           rtt_stats_.SmoothedRtt().Multiply(1.5));
@@ skipping 69 matching lines @@
     return;
   }
 
   using_pacing_ = true;
   send_algorithm_.reset(
       new PacingSender(send_algorithm_.release(),
                        QuicTime::Delta::FromMicroseconds(1)));
 }
 
 }  // namespace net