Skip to content

Commit

Permalink
Revert 226390 "Land Recent QUIC changes."
Browse files Browse the repository at this point in the history 
> Land Recent QUIC changes.
> 
> Fixing minor nits (added using std::pair).
> 
> Merge internal change: 53257699
> 
> Increasing the maximum tcp congestion window to 200 packets from 100.
> 
> Merge internal change: 53185276
> 
> Add a DCHECK to ensure the sent packet sequence number never goes down
> and separate the send alarm into a send alarm and a resume writes
> alarm to be used when the socket is still writable an there may be
> more pending data to write.
> 
> Merge internal change: 53050471
> 
> Merged quic_epoll_connection_helper_test.cc from internal code.
> 
> Minor nit fixes.
> 
> Merge internal change: 53048224
> 
> Move QuicAckNotifierManager from QuicConnection to
> QuicSentPacketManager.
> 
> Merge internal change: 53036185
> 
> R=jar@chromium.org, rch@chromium.org, wtc@chromium.org
> 
> Review URL: https://codereview.chromium.org/25443002

TBR=rtenneti@chromium.org
BUG=303981

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@227044 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
rtenneti@chromium.org committed Oct 4, 2013
1 parent 13621d6 commit a058f3d
Show file tree
Hide file tree
Showing 9 changed files with 89 additions and 118 deletions.
8 changes: 3 additions & 5 deletions net/quic/congestion_control/inter_arrival_sender_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@
#include "net/quic/test_tools/mock_clock.h"
#include "testing/gtest/include/gtest/gtest.h"

using std::pair;

namespace net {
namespace test {

Expand Down Expand Up @@ -63,14 +61,14 @@ class InterArrivalSenderTest : public ::testing::Test {
receive_clock_.AdvanceTime(spike_time);
QuicTime receive_time = receive_clock_.ApproximateNow();
feedback.inter_arrival.received_packet_times.insert(
pair<QuicPacketSequenceNumber, QuicTime>(
std::pair<QuicPacketSequenceNumber, QuicTime>(
feedback_sequence_number_, receive_time));
feedback_sequence_number_++;

// We need to send feedback for 2 packets since they where sent at the
// same time.
feedback.inter_arrival.received_packet_times.insert(
pair<QuicPacketSequenceNumber, QuicTime>(
std::pair<QuicPacketSequenceNumber, QuicTime>(
feedback_sequence_number_, receive_time));
feedback_sequence_number_++;

Expand All @@ -92,7 +90,7 @@ class InterArrivalSenderTest : public ::testing::Test {
}
QuicTime receive_time = receive_clock_.ApproximateNow();
feedback.inter_arrival.received_packet_times.insert(
pair<QuicPacketSequenceNumber, QuicTime>(
std::pair<QuicPacketSequenceNumber, QuicTime>(
feedback_sequence_number_, receive_time));
feedback_sequence_number_++;
}
Expand Down
4 changes: 3 additions & 1 deletion net/quic/congestion_control/send_algorithm_interface.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,10 @@
namespace net {

const bool kUseReno = false;
// TODO(ianswett): Increase the max congestion window once the RTO logic is
// improved, particularly in cases when RTT is larger than the RTO. b/10075719
// Maximum number of outstanding packets for tcp.
const QuicTcpCongestionWindow kMaxTcpCongestionWindow = 200;
const QuicTcpCongestionWindow kMaxTcpCongestionWindow = 100;

// Factory for send side congestion control algorithm.
SendAlgorithmInterface* SendAlgorithmInterface::Create(
Expand Down
49 changes: 24 additions & 25 deletions net/quic/quic_ack_notifier_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <stddef.h>
#include <list>
#include <map>
#include <set>
#include <utility>
#include <vector>

Expand Down Expand Up @@ -45,13 +46,13 @@ void AckNotifierManager::OnIncomingAck(const SequenceNumberSet& acked_packets) {
}

// Clear up any empty AckNotifiers
AckNotifierSet::iterator it = ack_notifiers_.begin();
AckNotifierList::iterator it = ack_notifiers_.begin();
while (it != ack_notifiers_.end()) {
if ((*it)->IsEmpty()) {
// The QuicAckNotifier has seen all the ACKs it was interested in, and
// has triggered its callback. No more use for it.
delete *it;
ack_notifiers_.erase(it++);
it = ack_notifiers_.erase(it);
} else {
++it;
}
Expand Down Expand Up @@ -83,31 +84,29 @@ void AckNotifierManager::OnSerializedPacket(
// Run through all the frames and if any of them are stream frames and have
// an AckNotifier registered, then inform the AckNotifier that it should be
// interested in this packet's sequence number.

RetransmittableFrames* frames = serialized_packet.retransmittable_frames;

// AckNotifiers can only be attached to retransmittable frames.
if (!frames) {
return;
}

for (QuicFrames::const_iterator it = frames->frames().begin();
it != frames->frames().end(); ++it) {
if (it->type == STREAM_FRAME && it->stream_frame->notifier != NULL) {
// The AckNotifier needs to know it is tracking this packet's sequence
// number.
it->stream_frame->notifier->AddSequenceNumber(
serialized_packet.sequence_number);

// Add the AckNotifier to our set of AckNotifiers.
ack_notifiers_.insert(it->stream_frame->notifier);

// Update the mapping in the other direction, from sequence
// number to AckNotifier.
ack_notifier_map_[serialized_packet.sequence_number]
.insert(it->stream_frame->notifier);
RetransmittableFrames* retransmittable_frames =
serialized_packet.retransmittable_frames;
if (retransmittable_frames) {
for (QuicFrames::const_iterator it =
retransmittable_frames->frames().begin();
it != retransmittable_frames->frames().end(); ++it) {
if (it->type == STREAM_FRAME && it->stream_frame->notifier != NULL) {
// The AckNotifier needs to know it is tracking this packet's sequence
// number.
it->stream_frame->notifier->AddSequenceNumber(
serialized_packet.sequence_number);

// Update the mapping in the other direction, from sequence
// number to AckNotifier.
ack_notifier_map_[serialized_packet.sequence_number]
.insert(it->stream_frame->notifier);
}
}
}
}

void AckNotifierManager::AddAckNotifier(QuicAckNotifier* notifier) {
ack_notifiers_.push_back(notifier);
}

} // namespace net
52 changes: 23 additions & 29 deletions net/quic/quic_ack_notifier_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,30 +5,20 @@
#ifndef NET_QUIC_QUIC_ACK_NOTIFIER_MANAGER_H_
#define NET_QUIC_QUIC_ACK_NOTIFIER_MANAGER_H_

#include <list>
#include <map>
#include <set>

#include "base/containers/hash_tables.h"
#include "net/quic/quic_protocol.h"

#if defined(COMPILER_GCC)
namespace BASE_HASH_NAMESPACE {
template<>
struct hash<net::QuicAckNotifier*> {
std::size_t operator()(const net::QuicAckNotifier* ptr) const {
return hash<size_t>()(reinterpret_cast<size_t>(ptr));
}
};
}
#endif

namespace net {

class QuicAckNotifier;

// The AckNotifierManager is used by the QuicSentPacketManager to keep track of
// all the AckNotifiers currently active. It owns the AckNotifiers which it gets
// from the serialized packets passed into OnSerializedPacket. It maintains both
// a set of AckNotifiers and a map from sequence number to AckNotifier the sake
// The AckNotifierManager is used by the QuicConnection to keep track of all the
// AckNotifiers currently active. It owns the AckNotifiers which it gets from
// the serialized packets passed into OnSerializedPacket. It maintains both a
// list of AckNotifiers and a map from sequence number to AckNotifier the sake
// of efficiency - we can quickly check the map to see if any AckNotifiers are
// interested in a given sequence number.

Expand All @@ -37,41 +27,45 @@ class NET_EXPORT_PRIVATE AckNotifierManager {
AckNotifierManager();
virtual ~AckNotifierManager();

// Called when the connection receives a new AckFrame. For each packet
// Called from QuicConnection when it receives a new AckFrame. For each packet
// in |acked_packets|, if the packet sequence number exists in
// ack_notifier_map_ then the corresponding AckNotifiers will have their OnAck
// method called.
void OnIncomingAck(const SequenceNumberSet& acked_packets);

// If a packet has been retransmitted with a new sequence number, then this
// will be called. It updates the mapping in ack_notifier_map_, and also
// updates the internal set of sequence numbers in each matching AckNotifier.
// updates the internal list of sequence numbers in each matching AckNotifier.
void UpdateSequenceNumber(QuicPacketSequenceNumber old_sequence_number,
QuicPacketSequenceNumber new_sequence_number);

// This is called after a packet has been serialized, is ready to be sent, and
// contains retransmittable frames (which may have associated AckNotifiers).
// If any of the retransmittable frames included in |serialized_packet| have
// AckNotifiers registered, then add them to our internal map and additionally
// inform the AckNotifier of the sequence number which it should track.
// This is called after a packet has been serialized and is ready to be sent.
// If any of the frames in |serialized_packet| have AckNotifiers registered,
// then add them to our internal map and additionally inform the AckNotifier
// of the sequence number which it should track.
void OnSerializedPacket(const SerializedPacket& serialized_packet);

// Called from QuicConnection when data is sent which the sender would like to
// be notified on receipt of all ACKs. Adds the |notifier| to our map.
void AddAckNotifier(QuicAckNotifier* notifier);

private:
typedef base::hash_set<QuicAckNotifier*> AckNotifierSet;
typedef std::list<QuicAckNotifier*> AckNotifierList;
typedef std::set<QuicAckNotifier*> AckNotifierSet;
typedef std::map<QuicPacketSequenceNumber, AckNotifierSet> AckNotifierMap;

// On every ACK frame received by the connection, all the ack_notifiers_ will
// On every ACK frame received by this connection, all the ack_notifiers_ will
// be told which sequeunce numbers were ACKed.
// Once a given QuicAckNotifier has seen all the sequence numbers it is
// interested in, it will be deleted, and removed from this set.
// Owns the AckNotifiers in this set.
AckNotifierSet ack_notifiers_;
// interested in, it will be deleted, and removed from this list.
// Owns the AckNotifiers in this list.
AckNotifierList ack_notifiers_;

// Maps from sequence number to the AckNotifiers which are registered
// for that sequence number. On receipt of an ACK for a given sequence
// number, call OnAck for all mapped AckNotifiers.
// Does not own the AckNotifiers.
AckNotifierMap ack_notifier_map_;
std::map<QuicPacketSequenceNumber, AckNotifierSet> ack_notifier_map_;
};

} // namespace net
Expand Down
39 changes: 22 additions & 17 deletions net/quic/quic_connection.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "base/stl_util.h"
#include "net/quic/crypto/quic_decrypter.h"
#include "net/quic/crypto/quic_encrypter.h"
#include "net/quic/quic_ack_notifier_manager.h"
#include "net/quic/quic_bandwidth.h"
#include "net/quic/quic_utils.h"

Expand Down Expand Up @@ -220,7 +221,6 @@ QuicConnection::QuicConnection(QuicGuid guid,
retransmission_alarm_(helper->CreateAlarm(new RetransmissionAlarm(this))),
abandon_fec_alarm_(helper->CreateAlarm(new AbandonFecAlarm(this))),
send_alarm_(helper->CreateAlarm(new SendAlarm(this))),
resume_writes_alarm_(helper->CreateAlarm(new SendAlarm(this))),
timeout_alarm_(helper->CreateAlarm(new TimeoutAlarm(this))),
debug_visitor_(NULL),
packet_creator_(guid_, &framer_, random_generator_, is_server),
Expand All @@ -231,7 +231,6 @@ QuicConnection::QuicConnection(QuicGuid guid,
creation_time_(clock_->ApproximateNow()),
time_of_last_received_packet_(clock_->ApproximateNow()),
time_of_last_sent_packet_(clock_->ApproximateNow()),
sequence_number_of_last_inorder_packet_(0),
congestion_manager_(clock_, kTCP),
sent_packet_manager_(is_server, this),
version_negotiation_state_(START_NEGOTIATION),
Expand Down Expand Up @@ -523,6 +522,9 @@ void QuicConnection::ProcessAckFrame(const QuicAckFrame& incoming_ack) {
sent_packet_manager_.OnIncomingAck(
incoming_ack.received_info, received_truncated_ack_, &acked_packets);
if (acked_packets.size() > 0) {
// The AckNotifierManager should be informed of every ACKed sequence number.
ack_notifier_manager_.OnIncomingAck(acked_packets);

// Reset the RTO timeout for each packet when an ack is received.
if (retransmission_alarm_->IsSet()) {
retransmission_alarm_->Cancel();
Expand Down Expand Up @@ -916,7 +918,12 @@ QuicConsumedData QuicConnection::SendvStreamDataAndNotifyWhenAcked(
QuicConsumedData consumed_data =
SendvStreamDataInner(id, iov, iov_count, offset, fin, notifier);

if (consumed_data.bytes_consumed == 0) {
if (consumed_data.bytes_consumed > 0) {
// If some data was consumed, then the delegate should be registered for
// notification when the data is ACKed.
ack_notifier_manager_.AddAckNotifier(notifier);
DLOG(INFO) << "Registered AckNotifier.";
} else {
// No data was consumed, delete the notifier.
delete notifier;
}
Expand Down Expand Up @@ -1020,13 +1027,14 @@ bool QuicConnection::DoWrite() {
// blocked or the congestion manager to prohibit sending, so check again.
pending_handshake = visitor_->HasPendingHandshake() ? IS_HANDSHAKE
: NOT_HANDSHAKE;
if (!all_bytes_written && !resume_writes_alarm_->IsSet() &&
if (!write_blocked_ && !all_bytes_written &&
CanWrite(NOT_RETRANSMISSION, HAS_RETRANSMITTABLE_DATA,
pending_handshake)) {
// We're not write blocked, but some stream didn't write out all of its
// bytes. Register for 'immediate' resumption so we'll keep writing after
// other quic connections have had a chance to use the socket.
resume_writes_alarm_->Set(clock_->ApproximateNow());
send_alarm_->Cancel();
send_alarm_->Set(clock_->ApproximateNow());
}
}

Expand Down Expand Up @@ -1092,6 +1100,11 @@ void QuicConnection::WritePendingRetransmissions() {
pending.retransmittable_frames.frames(),
pending.sequence_number_length);

// A notifier may be waiting to hear about ACKs for the original sequence
// number. Inform them that the sequence number has changed.
ack_notifier_manager_.UpdateSequenceNumber(
pending.sequence_number, serialized_packet.sequence_number);

DLOG(INFO) << ENDPOINT << "Retransmitting " << pending.sequence_number
<< " as " << serialized_packet.sequence_number;
if (debug_visitor_) {
Expand Down Expand Up @@ -1173,8 +1186,8 @@ bool QuicConnection::ShouldGeneratePacket(
bool QuicConnection::CanWrite(TransmissionType transmission_type,
HasRetransmittableData retransmittable,
IsHandshake handshake) {
// This check assumes that if the send alarm is set, it applies equally to all
// types of transmissions.
// TODO(ianswett): If the packet is a retransmit, the current send alarm may
// be too long.
if (write_blocked_ || send_alarm_->IsSet()) {
return false;
}
Expand Down Expand Up @@ -1305,16 +1318,6 @@ bool QuicConnection::WritePacket(EncryptionLevel level,
return false;
}

// Some encryption algorithms require the packet sequence numbers not be
// repeated.
DCHECK_LE(sequence_number_of_last_inorder_packet_, sequence_number);
// Only increase this when packets have not been queued. Once they're queued
// due to a write block, there is the chance of sending forced and other
// higher priority packets out of order.
if (queued_packets_.empty()) {
sequence_number_of_last_inorder_packet_ = sequence_number;
}

scoped_ptr<QuicEncryptedPacket> encrypted(
framer_.EncryptPacket(level, sequence_number, *packet));
if (encrypted.get() == NULL) {
Expand Down Expand Up @@ -1413,6 +1416,8 @@ int QuicConnection::WritePacketToWire(QuicPacketSequenceNumber sequence_number,

bool QuicConnection::OnSerializedPacket(
const SerializedPacket& serialized_packet) {
ack_notifier_manager_.OnSerializedPacket(serialized_packet);

if (serialized_packet.retransmittable_frames) {
serialized_packet.retransmittable_frames->
set_encryption_level(encryption_level_);
Expand Down
8 changes: 0 additions & 8 deletions net/quic/quic_connection.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -722,9 +722,6 @@ class NET_EXPORT_PRIVATE QuicConnection
// An alarm that is scheduled when the sent scheduler requires a
// a delay before sending packets and fires when the packet may be sent.
scoped_ptr<QuicAlarm> send_alarm_;
// An alarm that is scheduled when the connection can still write and there
// may be more data to send.
scoped_ptr<QuicAlarm> resume_writes_alarm_;
// An alarm that fires when the connection may have timed out.
scoped_ptr<QuicAlarm> timeout_alarm_;

Expand All @@ -750,11 +747,6 @@ class NET_EXPORT_PRIVATE QuicConnection
// The time that we last sent a packet for this connection.
QuicTime time_of_last_sent_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
// re-ordering.
QuicPacketSequenceNumber sequence_number_of_last_inorder_packet_;

// Congestion manager which controls the rate the connection sends packets
// as well as collecting and generating congestion feedback.
QuicCongestionManager congestion_manager_;
Expand Down
14 changes: 0 additions & 14 deletions net/quic/quic_sent_packet_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

#include "base/logging.h"
#include "base/stl_util.h"
#include "net/quic/quic_ack_notifier_manager.h"

using std::make_pair;

Expand Down Expand Up @@ -59,8 +58,6 @@ void QuicSentPacketManager::OnSerializedPacket(
return;
}

ack_notifier_manager_.OnSerializedPacket(serialized_packet);

DCHECK(unacked_packets_.empty() ||
unacked_packets_.rbegin()->first <
serialized_packet.sequence_number);
Expand Down Expand Up @@ -91,12 +88,6 @@ void QuicSentPacketManager::OnRetransmittedPacket(

RetransmittableFrames* frames = unacked_packets_[old_sequence_number];
DCHECK(frames);

// A notifier may be waiting to hear about ACKs for the original sequence
// number. Inform them that the sequence number has changed.
ack_notifier_manager_.UpdateSequenceNumber(old_sequence_number,
new_sequence_number);

if (FLAGS_track_retransmission_history) {
// We keep the old packet in the unacked packet list until it, or one of
// the retransmissions of it are acked.
Expand Down Expand Up @@ -135,11 +126,6 @@ void QuicSentPacketManager::OnIncomingAck(
SequenceNumberSet* acked_packets) {
HandleAckForSentPackets(received_info, is_truncated_ack, acked_packets);
HandleAckForSentFecPackets(received_info, acked_packets);

if (acked_packets->size() > 0) {
// The AckNotifierManager should be informed of every ACKed sequence number.
ack_notifier_manager_.OnIncomingAck(*acked_packets);
}
}

void QuicSentPacketManager::DiscardUnackedPacket(
Expand Down
Loading

0 comments on commit a058f3d

Please sign in to comment.