Skip to content

Commit

Permalink
Land Recent QUIC Changes.
Browse files Browse the repository at this point in the history
Slight improvement to QUIC version negotation debug logging.

Merge internal change: 73319950
https://codereview.chromium.org/477813003/


Improve QUIC's SentEntropyManagerTest and removing a test from
QuicConnectionTest that tests no code in QuicConnection.

Merge internal change: 73298415
https://codereview.chromium.org/479463002/


quic_session.h doesn't need to include util/gtl/linked_hash_map.h.

Merge internal change: 73256287
https://codereview.chromium.org/477863003/


Fix comment for QUIC handshake message tag COPT.

Merge internal change: 73256108
https://codereview.chromium.org/474133002/


Not used in production. Add InRecovery() method to QUIC send algorithm
interface, and pass slow start bool to bandwidth recorder.

We still want to generate bandwidth estimates while in slow start, and
include this information in the STK proto. We also need to know when we
are in recovery mode so we can avoid calculating estimates.

Merge internal change: 73239536
https://codereview.chromium.org/479443002/


Add a flag to enable QUIC's BBR congestion control by default instead of
Cubic in order to test and gather logs on internal servers.

Merge internal change: 73146298
https://codereview.chromium.org/477863002/


Add a QuicSustainedBandwidthEstimator class. Not yet used.

Followup change list in progress (not ready for review, but in case you
want to see intended usage) which does the following:

1) Use this in the QuicSentPacketManager.
2) Detect substantial differences between subsequent sustained bandwidth
   estimates.
3) Send estimates to client in server config update.

internal change list: 72831442

Merge internal change: 73132177
https://codereview.chromium.org/477053002/


Re-add the QUIC DCHECK to ensure packets are always sent in the order of
increasing sequence number.

Merge internal change: 73128051
https://codereview.chromium.org/469383004/


Change to optimize QUIC's RetransmittableFrames::HasCryptoHandshake by
storing it once when the crypto frame is added to RetransmittableFrames.

Merge internal change: 73125935
https://codereview.chromium.org/467263003/


Add an Update method on QuicAlarm which contains a granularity.  Also
change the idle network timeout to be 1 second shorter on the client and
1 second longer on the server.

Change saves ~2% of CPU according to an EndToEndTest profile.

Merge internal change: 73125905
https://codereview.chromium.org/471313002/

R=rch@chromium.org

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

Cr-Commit-Position: refs/heads/master@{#290106}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@290106 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
rtenneti@chromium.org committed Aug 16, 2014
1 parent e8ef876 commit 672631c
Show file tree
Hide file tree
Showing 31 changed files with 537 additions and 109 deletions.
22 changes: 22 additions & 0 deletions net/base/linked_hash_map.h
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,28 @@ class linked_hash_map {
return list_.rend();
}

// Front and back accessors common to many stl containers.

// Returns the earliest-inserted element
const value_type& front() const {
return list_.front();
}

// Returns the earliest-inserted element.
value_type& front() {
return list_.front();
}

// Returns the most-recently-inserted element.
const value_type& back() const {
return list_.back();
}

// Returns the most-recently-inserted element.
value_type& back() {
return list_.back();
}

// Clears the map of all values.
void clear() {
map_.clear();
Expand Down
3 changes: 3 additions & 0 deletions net/net.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -925,6 +925,8 @@
'quic/quic_stream_factory.h',
'quic/quic_stream_sequencer.cc',
'quic/quic_stream_sequencer.h',
'quic/quic_sustained_bandwidth_recorder.cc',
'quic/quic_sustained_bandwidth_recorder.h',
'quic/quic_time.cc',
'quic/quic_time.h',
'quic/quic_types.cc',
Expand Down Expand Up @@ -1549,6 +1551,7 @@
'quic/quic_socket_address_coder_test.cc',
'quic/quic_stream_factory_test.cc',
'quic/quic_stream_sequencer_test.cc',
'quic/quic_sustained_bandwidth_recorder_test.cc',
'quic/quic_time_test.cc',
'quic/quic_unacked_packet_map_test.cc',
'quic/quic_utils_chromium_test.cc',
Expand Down
4 changes: 4 additions & 0 deletions net/quic/congestion_control/pacing_sender.cc
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,10 @@ bool PacingSender::InSlowStart() const {
return sender_->InSlowStart();
}

bool PacingSender::InRecovery() const {
return sender_->InRecovery();
}

QuicByteCount PacingSender::GetSlowStartThreshold() const {
return sender_->GetSlowStartThreshold();
}
Expand Down
1 change: 1 addition & 0 deletions net/quic/congestion_control/pacing_sender.h
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ class NET_EXPORT_PRIVATE PacingSender : public SendAlgorithmInterface {
virtual QuicTime::Delta RetransmissionDelay() const OVERRIDE;
virtual QuicByteCount GetCongestionWindow() const OVERRIDE;
virtual bool InSlowStart() const OVERRIDE;
virtual bool InRecovery() const OVERRIDE;
virtual QuicByteCount GetSlowStartThreshold() const OVERRIDE;
virtual CongestionControlType GetCongestionControlType() const OVERRIDE;

Expand Down
3 changes: 3 additions & 0 deletions net/quic/congestion_control/send_algorithm_interface.h
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,9 @@ class NET_EXPORT_PRIVATE SendAlgorithmInterface {
// BandwidthEstimate is expected to be too low.
virtual bool InSlowStart() const = 0;

// Whether the send algorithm is currently in recovery.
virtual bool InRecovery() const = 0;

// Returns the size of the slow start congestion window in bytes,
// aka ssthresh. Some send algorithms do not define a slow start
// threshold and will return 0.
Expand Down
7 changes: 2 additions & 5 deletions net/quic/congestion_control/tcp_cubic_sender.cc
Original file line number Diff line number Diff line change
Expand Up @@ -170,11 +170,8 @@ bool TcpCubicSender::OnPacketSent(QuicTime /*sent_time*/,
}

prr_out_ += bytes;
if (largest_sent_sequence_number_ < sequence_number) {
// TODO(rch): Ensure that packets are really sent in order.
// DCHECK_LT(largest_sent_sequence_number_, sequence_number);
largest_sent_sequence_number_ = sequence_number;
}
DCHECK_LT(largest_sent_sequence_number_, sequence_number);
largest_sent_sequence_number_ = sequence_number;
hybrid_slow_start_.OnPacketSent(sequence_number);
return true;
}
Expand Down
2 changes: 1 addition & 1 deletion net/quic/congestion_control/tcp_cubic_sender.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ class NET_EXPORT_PRIVATE TcpCubicSender : public SendAlgorithmInterface {
virtual QuicTime::Delta RetransmissionDelay() const OVERRIDE;
virtual QuicByteCount GetCongestionWindow() const OVERRIDE;
virtual bool InSlowStart() const OVERRIDE;
virtual bool InRecovery() const OVERRIDE;
virtual QuicByteCount GetSlowStartThreshold() const OVERRIDE;
virtual CongestionControlType GetCongestionControlType() const OVERRIDE;
// End implementation of SendAlgorithmInterface.
Expand All @@ -80,7 +81,6 @@ class NET_EXPORT_PRIVATE TcpCubicSender : public SendAlgorithmInterface {
void MaybeIncreaseCwnd(QuicPacketSequenceNumber acked_sequence_number,
QuicByteCount bytes_in_flight);
bool IsCwndLimited(QuicByteCount bytes_in_flight) const;
bool InRecovery() const;
// Methods for isolating PRR from the rest of TCP Cubic.
void PrrOnPacketLost(QuicByteCount bytes_in_flight);
void PrrOnPacketAcked(QuicByteCount acked_bytes);
Expand Down
4 changes: 2 additions & 2 deletions net/quic/crypto/crypto_protocol.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ const QuicTag kSRBF = TAG('S', 'R', 'B', 'F'); // Socket receive buffer
const QuicTag kQBIC = TAG('Q', 'B', 'I', 'C'); // TCP cubic
const QuicTag kTSTP = TAG('T', 'S', 'T', 'P'); // Timestamp

// Congestion control options
// Connection options (COPT) values
const QuicTag kTBBR = TAG('T', 'B', 'B', 'R'); // Reduced Buffer Bloat TCP
const QuicTag kRENO = TAG('R', 'E', 'N', 'O'); // Reno Congestion Control
const QuicTag kIW10 = TAG('I', 'W', '1', '0'); // Force ICWND to 10
Expand Down Expand Up @@ -81,7 +81,7 @@ const QuicTag kAEAD = TAG('A', 'E', 'A', 'D'); // Authenticated
// encryption algorithms
const QuicTag kCGST = TAG('C', 'G', 'S', 'T'); // Congestion control
// feedback types
const QuicTag kCOPT = TAG('C', 'O', 'P', 'T'); // Congestion control options
const QuicTag kCOPT = TAG('C', 'O', 'P', 'T'); // Connection options
// kLOSS was 'L', 'O', 'S', 'S', but was changed from a tag vector to a tag.
const QuicTag kLOSS = TAG('L', 'O', 'S', 'A'); // Loss detection algorithms
const QuicTag kICSL = TAG('I', 'C', 'S', 'L'); // Idle connection state
Expand Down
13 changes: 13 additions & 0 deletions net/quic/quic_alarm.cc
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,19 @@ void QuicAlarm::Cancel() {
CancelImpl();
}

void QuicAlarm::Update(QuicTime deadline, QuicTime::Delta granularity) {
if (!deadline.IsInitialized()) {
Cancel();
return;
}
if (std::abs(deadline.Subtract(deadline_).ToMicroseconds()) <
granularity.ToMicroseconds()) {
return;
}
Cancel();
Set(deadline);
}

bool QuicAlarm::IsSet() const {
return deadline_.IsInitialized();
}
Expand Down
5 changes: 5 additions & 0 deletions net/quic/quic_alarm.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ class NET_EXPORT_PRIVATE QuicAlarm {
// delegates OnAlarm method will not be called.
void Cancel();

// Cancels and sets the alarm if the |deadline| is farther from the current
// deadline than |granularity|, and otherwise does nothing. If |deadline| is
// not initialized, the alarm is cancelled.
void Update(QuicTime deadline, QuicTime::Delta granularity);

bool IsSet() const;

QuicTime deadline() const { return deadline_; }
Expand Down
19 changes: 19 additions & 0 deletions net/quic/quic_alarm_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,25 @@ TEST_F(QuicAlarmTest, Cancel) {
EXPECT_EQ(QuicTime::Zero(), alarm_.deadline());
}

TEST_F(QuicAlarmTest, Update) {
QuicTime deadline = QuicTime::Zero().Add(QuicTime::Delta::FromSeconds(7));
alarm_.Set(deadline);
QuicTime new_deadline = QuicTime::Zero().Add(QuicTime::Delta::FromSeconds(8));
alarm_.Update(new_deadline, QuicTime::Delta::Zero());
EXPECT_TRUE(alarm_.IsSet());
EXPECT_TRUE(alarm_.scheduled());
EXPECT_EQ(new_deadline, alarm_.deadline());
}

TEST_F(QuicAlarmTest, UpdateWithZero) {
QuicTime deadline = QuicTime::Zero().Add(QuicTime::Delta::FromSeconds(7));
alarm_.Set(deadline);
alarm_.Update(QuicTime::Zero(), QuicTime::Delta::Zero());
EXPECT_FALSE(alarm_.IsSet());
EXPECT_FALSE(alarm_.scheduled());
EXPECT_EQ(QuicTime::Zero(), alarm_.deadline());
}

TEST_F(QuicAlarmTest, Fire) {
QuicTime deadline = QuicTime::Zero().Add(QuicTime::Delta::FromSeconds(7));
alarm_.Set(deadline);
Expand Down
48 changes: 28 additions & 20 deletions net/quic/quic_connection.cc
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,8 @@ QuicConnection::QuicConnection(QuicConnectionId connection_id,
time_of_last_sent_new_packet_(clock_->ApproximateNow()),
sequence_number_of_last_sent_packet_(0),
sent_packet_manager_(
is_server, clock_, &stats_, kCubic,
is_server, clock_, &stats_,
FLAGS_quic_use_bbr_congestion_control ? kBBR : kCubic,
FLAGS_quic_use_time_loss_detection ? kTime : kNack),
version_negotiation_state_(START_NEGOTIATION),
is_server_(is_server),
Expand Down Expand Up @@ -408,7 +409,8 @@ void QuicConnection::OnVersionNegotiationPacket(
return;
}

DVLOG(1) << ENDPOINT << "negotiating version " << version();
DVLOG(1) << ENDPOINT
<< "Negotiated version: " << QuicVersionToString(version());
server_supported_versions_ = packet.versions;
version_negotiation_state_ = NEGOTIATION_IN_PROGRESS;
RetransmitUnackedPackets(ALL_PACKETS);
Expand Down Expand Up @@ -569,12 +571,10 @@ void QuicConnection::ProcessAckFrame(const QuicAckFrame& incoming_ack) {

// Always reset the retransmission alarm when an ack comes in, since we now
// have a better estimate of the current rtt than when it was set.
retransmission_alarm_->Cancel();
QuicTime retransmission_time =
sent_packet_manager_.GetRetransmissionTime();
if (retransmission_time != QuicTime::Zero()) {
retransmission_alarm_->Set(retransmission_time);
}
retransmission_alarm_->Update(retransmission_time,
QuicTime::Delta::FromMilliseconds(1));
}

void QuicConnection::ProcessStopWaitingFrame(
Expand Down Expand Up @@ -970,6 +970,8 @@ void QuicConnection::SendVersionNegotiationPacket() {
visitor_->OnWriteBlocked();
return;
}
DVLOG(1) << ENDPOINT << "Sending version negotiation packet: {"
<< QuicVersionVectorToString(framer_.supported_versions()) << "}";
scoped_ptr<QuicEncryptedPacket> version_packet(
packet_generator_.SerializeVersionNegotiationPacket(
framer_.supported_versions()));
Expand Down Expand Up @@ -1278,11 +1280,9 @@ void QuicConnection::RetransmitUnackedPackets(
void QuicConnection::NeuterUnencryptedPackets() {
sent_packet_manager_.NeuterUnencryptedPackets();
// This may have changed the retransmission timer, so re-arm it.
retransmission_alarm_->Cancel();
QuicTime retransmission_time = sent_packet_manager_.GetRetransmissionTime();
if (retransmission_time != QuicTime::Zero()) {
retransmission_alarm_->Set(retransmission_time);
}
retransmission_alarm_->Update(retransmission_time,
QuicTime::Delta::FromMilliseconds(1));
}

bool QuicConnection::ShouldGeneratePacket(
Expand All @@ -1304,20 +1304,21 @@ bool QuicConnection::CanWrite(HasRetransmittableData retransmittable) {
return false;
}

send_alarm_->Cancel();
QuicTime now = clock_->Now();
QuicTime::Delta delay = sent_packet_manager_.TimeUntilSend(
now, retransmittable);
if (delay.IsInfinite()) {
send_alarm_->Cancel();
return false;
}

// If the scheduler requires a delay, then we can not send this packet now.
if (!delay.IsZero()) {
send_alarm_->Set(now.Add(delay));
send_alarm_->Update(now.Add(delay), QuicTime::Delta::FromMilliseconds(1));
DVLOG(1) << "Delaying sending.";
return false;
}
send_alarm_->Cancel();
return true;
}

Expand Down Expand Up @@ -1511,11 +1512,9 @@ bool QuicConnection::OnPacketSent(WriteResult result) {
transmission_type, retransmittable);

if (reset_retransmission_alarm || !retransmission_alarm_->IsSet()) {
retransmission_alarm_->Cancel();
QuicTime retransmission_time = sent_packet_manager_.GetRetransmissionTime();
if (retransmission_time != QuicTime::Zero()) {
retransmission_alarm_->Set(retransmission_time);
}
retransmission_alarm_->Update(retransmission_time,
QuicTime::Delta::FromMilliseconds(1));
}

stats_.bytes_sent += result.bytes_written;
Expand Down Expand Up @@ -1632,7 +1631,7 @@ void QuicConnection::OnRetransmissionTimeout() {
// and nothing waiting to be sent.
if (!HasQueuedData() && !retransmission_alarm_->IsSet()) {
QuicTime rto_timeout = sent_packet_manager_.GetRetransmissionTime();
if (rto_timeout != QuicTime::Zero()) {
if (rto_timeout.IsInitialized()) {
retransmission_alarm_->Set(rto_timeout);
}
}
Expand Down Expand Up @@ -1882,6 +1881,14 @@ bool QuicConnection::CanWriteStreamData() {
}

void QuicConnection::SetIdleNetworkTimeout(QuicTime::Delta timeout) {
// Adjust the idle timeout on client and server to prevent clients from
// sending requests to servers which have already closed the connection.
if (is_server_) {
timeout = timeout.Add(QuicTime::Delta::FromSeconds(1));
} else if (timeout > QuicTime::Delta::FromSeconds(1)) {
timeout = timeout.Subtract(QuicTime::Delta::FromSeconds(1));
}

if (timeout < idle_network_timeout_) {
idle_network_timeout_ = timeout;
CheckForTimeout();
Expand Down Expand Up @@ -1944,7 +1951,7 @@ bool QuicConnection::CheckForTimeout() {
}

timeout_alarm_->Cancel();
timeout_alarm_->Set(clock_->ApproximateNow().Add(timeout));
timeout_alarm_->Set(now.Add(timeout));
return false;
}

Expand All @@ -1953,13 +1960,14 @@ void QuicConnection::SetPingAlarm() {
// Only clients send pings.
return;
}
ping_alarm_->Cancel();
if (!visitor_->HasOpenDataStreams()) {
ping_alarm_->Cancel();
// Don't send a ping unless there are open streams.
return;
}
QuicTime::Delta ping_timeout = QuicTime::Delta::FromSeconds(kPingTimeoutSecs);
ping_alarm_->Set(clock_->ApproximateNow().Add(ping_timeout));
ping_alarm_->Update(clock_->ApproximateNow().Add(ping_timeout),
QuicTime::Delta::FromSeconds(1));
}

QuicConnection::ScopedPacketBundler::ScopedPacketBundler(
Expand Down
45 changes: 45 additions & 0 deletions net/quic/quic_connection_helper_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,51 @@ TEST_F(QuicConnectionHelperTest, CreateAlarmAndResetEarlier) {
EXPECT_FALSE(delegate->fired());
}

TEST_F(QuicConnectionHelperTest, CreateAlarmAndUpdate) {
TestDelegate* delegate = new TestDelegate();
scoped_ptr<QuicAlarm> alarm(helper_.CreateAlarm(delegate));

const QuicClock* clock = helper_.GetClock();
QuicTime start = clock->Now();
QuicTime::Delta delta = QuicTime::Delta::FromMicroseconds(1);
alarm->Set(clock->Now().Add(delta));
QuicTime::Delta new_delta = QuicTime::Delta::FromMicroseconds(3);
alarm->Update(clock->Now().Add(new_delta),
QuicTime::Delta::FromMicroseconds(1));

// The alarm task should still be posted.
ASSERT_EQ(1u, runner_->GetPostedTasks().size());
EXPECT_EQ(base::TimeDelta::FromMicroseconds(delta.ToMicroseconds()),
runner_->GetPostedTasks()[0].delay);

runner_->RunNextTask();
EXPECT_EQ(QuicTime::Zero().Add(delta), clock->Now());
EXPECT_FALSE(delegate->fired());

// Move the alarm forward 1us and ensure it doesn't move forward.
alarm->Update(clock->Now().Add(new_delta),
QuicTime::Delta::FromMicroseconds(2));

ASSERT_EQ(1u, runner_->GetPostedTasks().size());
EXPECT_EQ(
base::TimeDelta::FromMicroseconds(
new_delta.Subtract(delta).ToMicroseconds()),
runner_->GetPostedTasks()[0].delay);
runner_->RunNextTask();
EXPECT_EQ(start.Add(new_delta), clock->Now());
EXPECT_TRUE(delegate->fired());

// Set the alarm via an update call.
new_delta = QuicTime::Delta::FromMicroseconds(5);
alarm->Update(clock->Now().Add(new_delta),
QuicTime::Delta::FromMicroseconds(1));
EXPECT_TRUE(alarm->IsSet());

// Update it with an uninitialized time and ensure it's cancelled.
alarm->Update(QuicTime::Zero(), QuicTime::Delta::FromMicroseconds(1));
EXPECT_FALSE(alarm->IsSet());
}

} // namespace
} // namespace test
} // namespace net
Loading

0 comments on commit 672631c

Please sign in to comment.