diff --git a/net/quic/congestion_control/inter_arrival_sender_test.cc b/net/quic/congestion_control/inter_arrival_sender_test.cc index c1c848bb52d4fc..16d4fa234cfd68 100644 --- a/net/quic/congestion_control/inter_arrival_sender_test.cc +++ b/net/quic/congestion_control/inter_arrival_sender_test.cc @@ -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 { @@ -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( + std::pair( 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( + std::pair( feedback_sequence_number_, receive_time)); feedback_sequence_number_++; @@ -92,7 +90,7 @@ class InterArrivalSenderTest : public ::testing::Test { } QuicTime receive_time = receive_clock_.ApproximateNow(); feedback.inter_arrival.received_packet_times.insert( - pair( + std::pair( feedback_sequence_number_, receive_time)); feedback_sequence_number_++; } diff --git a/net/quic/congestion_control/send_algorithm_interface.cc b/net/quic/congestion_control/send_algorithm_interface.cc index a6399ad53a8688..90399e7d05b80d 100644 --- a/net/quic/congestion_control/send_algorithm_interface.cc +++ b/net/quic/congestion_control/send_algorithm_interface.cc @@ -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( diff --git a/net/quic/quic_ack_notifier_manager.cc b/net/quic/quic_ack_notifier_manager.cc index 901d9d67045d7d..76515bacbb45e5 100644 --- a/net/quic/quic_ack_notifier_manager.cc +++ b/net/quic/quic_ack_notifier_manager.cc @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -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; } @@ -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 diff --git a/net/quic/quic_ack_notifier_manager.h b/net/quic/quic_ack_notifier_manager.h index 7fe9a460d70882..f3a7f63211452f 100644 --- a/net/quic/quic_ack_notifier_manager.h +++ b/net/quic/quic_ack_notifier_manager.h @@ -5,30 +5,20 @@ #ifndef NET_QUIC_QUIC_ACK_NOTIFIER_MANAGER_H_ #define NET_QUIC_QUIC_ACK_NOTIFIER_MANAGER_H_ +#include #include +#include -#include "base/containers/hash_tables.h" #include "net/quic/quic_protocol.h" -#if defined(COMPILER_GCC) -namespace BASE_HASH_NAMESPACE { -template<> -struct hash { - std::size_t operator()(const net::QuicAckNotifier* ptr) const { - return hash()(reinterpret_cast(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. @@ -37,7 +27,7 @@ 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. @@ -45,33 +35,37 @@ class NET_EXPORT_PRIVATE AckNotifierManager { // 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 AckNotifierSet; + typedef std::list AckNotifierList; + typedef std::set AckNotifierSet; typedef std::map 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 ack_notifier_map_; }; } // namespace net diff --git a/net/quic/quic_connection.cc b/net/quic/quic_connection.cc index 0744806bfbc03f..fdb5e0f10f4349 100644 --- a/net/quic/quic_connection.cc +++ b/net/quic/quic_connection.cc @@ -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" @@ -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), @@ -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), @@ -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(); @@ -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; } @@ -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()); } } @@ -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_) { @@ -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; } @@ -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 encrypted( framer_.EncryptPacket(level, sequence_number, *packet)); if (encrypted.get() == NULL) { @@ -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_); diff --git a/net/quic/quic_connection.h b/net/quic/quic_connection.h index 2f20351f410edf..7afe3ea5a65fdf 100644 --- a/net/quic/quic_connection.h +++ b/net/quic/quic_connection.h @@ -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 send_alarm_; - // An alarm that is scheduled when the connection can still write and there - // may be more data to send. - scoped_ptr resume_writes_alarm_; // An alarm that fires when the connection may have timed out. scoped_ptr timeout_alarm_; @@ -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_; diff --git a/net/quic/quic_sent_packet_manager.cc b/net/quic/quic_sent_packet_manager.cc index a9f26b88d74deb..b31899b8a57587 100644 --- a/net/quic/quic_sent_packet_manager.cc +++ b/net/quic/quic_sent_packet_manager.cc @@ -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; @@ -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); @@ -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. @@ -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( diff --git a/net/quic/quic_sent_packet_manager.h b/net/quic/quic_sent_packet_manager.h index 4987bccf3f9e2f..4c755b6e2d02ff 100644 --- a/net/quic/quic_sent_packet_manager.h +++ b/net/quic/quic_sent_packet_manager.h @@ -15,7 +15,6 @@ #include "base/containers/hash_tables.h" #include "net/base/linked_hash_map.h" -#include "net/quic/quic_ack_notifier_manager.h" #include "net/quic/quic_protocol.h" NET_EXPORT_PRIVATE extern bool FLAGS_track_retransmission_history; @@ -247,11 +246,6 @@ class NET_EXPORT_PRIVATE QuicSentPacketManager { HelperInterface* helper_; - // An AckNotifier can register to be informed when ACKs have been received for - // all packets that a given block of data was sent in. The AckNotifierManager - // maintains the currently active notifiers. - AckNotifierManager ack_notifier_manager_; - DISALLOW_COPY_AND_ASSIGN(QuicSentPacketManager); }; diff --git a/net/tools/quic/quic_epoll_connection_helper_test.cc b/net/tools/quic/quic_epoll_connection_helper_test.cc index 154c3936b699ee..a61813c9e4f411 100644 --- a/net/tools/quic/quic_epoll_connection_helper_test.cc +++ b/net/tools/quic/quic_epoll_connection_helper_test.cc @@ -30,7 +30,7 @@ namespace tools { namespace test { namespace { -const char kData[] = "foo"; +const char data1[] = "foo"; const bool kFromPeer = true; class TestConnectionHelper : public QuicEpollConnectionHelper { @@ -81,7 +81,7 @@ class QuicEpollConnectionHelperTest : public ::testing::Test { send_algorithm_(new testing::StrictMock), helper_(new TestConnectionHelper(0, &epoll_server_)), connection_(guid_, IPEndPoint(), helper_), - frame_(3, false, 0, kData) { + frame1_(3, false, 0, data1) { connection_.set_visitor(&visitor_); connection_.SetSendAlgorithm(send_algorithm_); epoll_server_.set_timeout_in_us(-1); @@ -97,18 +97,18 @@ class QuicEpollConnectionHelperTest : public ::testing::Test { QuicPacket* ConstructDataPacket(QuicPacketSequenceNumber number, QuicFecGroupNumber fec_group) { - QuicPacketHeader header; - header.public_header.version_flag = false; - header.public_header.reset_flag = false; - header.fec_flag = false; - header.entropy_flag = false; - header.packet_sequence_number = number; - header.is_in_fec_group = fec_group == 0 ? NOT_IN_FEC_GROUP : IN_FEC_GROUP; - header.fec_group = fec_group; + header_.public_header.version_flag = false; + header_.public_header.reset_flag = false; + header_.fec_flag = false; + header_.entropy_flag = false; + header_.packet_sequence_number = number; + header_.is_in_fec_group = fec_group == 0 ? NOT_IN_FEC_GROUP : IN_FEC_GROUP; + header_.fec_group = fec_group; QuicFrames frames; - frames.push_back(QuicFrame(&frame_)); - return framer_.BuildUnsizedDataPacket(header, frames).packet; + QuicFrame frame(&frame1_); + frames.push_back(frame); + return framer_.BuildUnsizedDataPacket(header_, frames).packet; } QuicGuid guid_; @@ -120,7 +120,8 @@ class QuicEpollConnectionHelperTest : public ::testing::Test { TestConnection connection_; testing::StrictMock visitor_; - QuicStreamFrame frame_; + QuicPacketHeader header_; + QuicStreamFrame frame1_; }; TEST_F(QuicEpollConnectionHelperTest, DISABLED_TestRTORetransmission) {