Skip to content

Commit

Permalink
Land Recent QUIC Changes
Browse files Browse the repository at this point in the history 
Set the delayed ack timer to 0ms intstead of 100ms for crypto handshake
packets.

This both provides a more accurate RTT estimate early on in a connection
and gives the server confidence the handshake has completed and it's
safe to increase the CWND further.

Merge internal change: 64134758
https://codereview.chromium.org/221373005/


Making framer capable of handling stream frame headers in packets
correctly when using FEC. Stream frame length is always written, even
for the last stream frame with FEC, to avoid boundary condition and
padding issues in the framer.

These changes are working towards getting FEC up and running correctly.

Merge internal change: 64080142
https://codereview.chromium.org/212223011/


Change to QUIC's TLP timer to set it based on the last pending packet,
even when that packet has no retransmittable frames.

Merge internal change: 64059923
https://codereview.chromium.org/220163008/


QUIC - cleanup changes fixed while sync'ing with internal source tree.

https://codereview.chromium.org/212523003/

R=rch@chromium.org

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@261606 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
rtenneti@chromium.org committed Apr 4, 2014
1 parent 15429ab commit 98a9d12
Show file tree
Hide file tree
Showing 22 changed files with 258 additions and 95 deletions.
6 changes: 4 additions & 2 deletions net/quic/congestion_control/inter_arrival_bitrate_ramp_up.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
#include "net/quic/congestion_control/cube_root.h"
#include "net/quic/quic_protocol.h"

using std::max;

namespace {
// The following constants are in 2^10 fractions of a second instead of ms to
// allow a 10 shift right to divide.
Expand Down Expand Up @@ -37,8 +39,8 @@ void InterArrivalBitrateRampUp::Reset(QuicBandwidth new_rate,
QuicBandwidth channel_estimate) {
epoch_ = clock_->ApproximateNow();
last_update_time_ = epoch_;
available_channel_estimate_ = std::max(new_rate, available_channel_estimate);
channel_estimate_ = std::max(channel_estimate, available_channel_estimate_);
available_channel_estimate_ = max(new_rate, available_channel_estimate);
channel_estimate_ = max(channel_estimate, available_channel_estimate_);

halfway_point_ = available_channel_estimate_.Add(
(channel_estimate_.Subtract(available_channel_estimate_)).Scale(0.5f));
Expand Down
4 changes: 3 additions & 1 deletion net/quic/congestion_control/inter_arrival_overuse_detector.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@

#include <algorithm>

using std::max;

// Initial noise variance, equal to a standard deviation of 1 millisecond.
static const float kInitialVarianceNoise = 1000000.0;

Expand Down Expand Up @@ -120,7 +122,7 @@ BandwidthUsage InterArrivalOveruseDetector::GetState(
int64 sigma_delta = sqrt(static_cast<double>(delta_variance_));
DetectSlope(sigma_delta);
DetectDrift(sigma_delta);
return std::max(slope_estimate_, delta_estimate_);
return max(slope_estimate_, delta_estimate_);
}

void InterArrivalOveruseDetector::UpdateFilter(QuicTime::Delta received_delta,
Expand Down
1 change: 0 additions & 1 deletion net/quic/crypto/quic_crypto_client_config.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
#include "net/quic/crypto/p256_key_exchange.h"
#include "net/quic/crypto/proof_verifier.h"
#include "net/quic/crypto/quic_encrypter.h"
#include "net/quic/quic_server_id.h"
#include "net/quic/quic_utils.h"

using base::StringPiece;
Expand Down
3 changes: 2 additions & 1 deletion net/quic/crypto/quic_crypto_server_config.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
using base::StringPiece;
using crypto::SecureHash;
using std::map;
using std::sort;
using std::string;
using std::vector;

Expand Down Expand Up @@ -767,7 +768,7 @@ void QuicCryptoServerConfig::SelectNewPrimaryConfig(
return;
}

std::sort(configs.begin(), configs.end(), ConfigPrimaryTimeLessThan);
sort(configs.begin(), configs.end(), ConfigPrimaryTimeLessThan);

Config* best_candidate = configs[0];

Expand Down
16 changes: 10 additions & 6 deletions net/quic/quic_connection.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -813,8 +813,13 @@ void QuicConnection::MaybeQueueAck() {
if (ack_alarm_->IsSet()) {
ack_queued_ = true;
} else {
ack_alarm_->Set(clock_->ApproximateNow().Add(
sent_packet_manager_.DelayedAckTime()));
// Send an ack much more quickly for crypto handshake packets.
QuicTime::Delta delayed_ack_time = sent_packet_manager_.DelayedAckTime();
if (last_stream_frames_.size() == 1 &&
last_stream_frames_[0].stream_id == kCryptoStreamId) {
delayed_ack_time = QuicTime::Delta::Zero();
}
ack_alarm_->Set(clock_->ApproximateNow().Add(delayed_ack_time));
DVLOG(1) << "Ack timer set; next packet or timer will trigger ACK.";
}
}
Expand Down Expand Up @@ -959,10 +964,9 @@ QuicConsumedData QuicConnection::SendStreamData(
notifier = new QuicAckNotifier(delegate);
}

// Opportunistically bundle an ack with every outgoing packet, unless it's the
// crypto stream.
ScopedPacketBundler ack_bundler(
this, id != kCryptoStreamId ? BUNDLE_PENDING_ACK : NO_ACK);
// Opportunistically bundle an ack with every outgoing packet.
// TODO(ianswett): Consider not bundling an ack when there is no encryption.
ScopedPacketBundler ack_bundler(this, BUNDLE_PENDING_ACK);
QuicConsumedData consumed_data =
packet_generator_.ConsumeData(id, data, offset, fin, notifier);

Expand Down
36 changes: 30 additions & 6 deletions net/quic/quic_connection_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2823,10 +2823,34 @@ TEST_P(QuicConnectionTest, LoopThroughSendingPackets) {
!kFin, NULL).bytes_consumed);
}

TEST_P(QuicConnectionTest, SendDelayedAckOnTimer) {
TEST_P(QuicConnectionTest, SendDelayedAck) {
QuicTime ack_time = clock_.ApproximateNow().Add(DefaultDelayedAckTime());
EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_));
EXPECT_FALSE(connection_.GetAckAlarm()->IsSet());
// Process a packet from the non-crypto stream.
frame1_.stream_id = 3;
ProcessPacket(1);
// Check if delayed ack timer is running for the expected interval.
EXPECT_TRUE(connection_.GetAckAlarm()->IsSet());
EXPECT_EQ(ack_time, connection_.GetAckAlarm()->deadline());
// Simulate delayed ack alarm firing.
connection_.GetAckAlarm()->Fire();
// Check that ack is sent and that delayed ack alarm is reset.
if (version() > QUIC_VERSION_15) {
EXPECT_EQ(2u, writer_->frame_count());
EXPECT_TRUE(writer_->stop_waiting());
} else {
EXPECT_EQ(1u, writer_->frame_count());
}
EXPECT_TRUE(writer_->ack());
EXPECT_FALSE(connection_.GetAckAlarm()->IsSet());
}

TEST_P(QuicConnectionTest, SendEarlyDelayedAckForCrypto) {
QuicTime ack_time = clock_.ApproximateNow();
EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_));
EXPECT_FALSE(connection_.GetAckAlarm()->IsSet());
// Process a packet from the crypto stream, which is frame1_'s default.
ProcessPacket(1);
// Check if delayed ack timer is running for the expected interval.
EXPECT_TRUE(connection_.GetAckAlarm()->IsSet());
Expand Down Expand Up @@ -2901,14 +2925,14 @@ TEST_P(QuicConnectionTest, SendDelayedAckOnOutgoingPacket) {
EXPECT_FALSE(connection_.GetAckAlarm()->IsSet());
}

TEST_P(QuicConnectionTest, DontSendDelayedAckOnOutgoingCryptoPacket) {
TEST_P(QuicConnectionTest, SendDelayedAckOnOutgoingCryptoPacket) {
EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_));
ProcessPacket(1);
connection_.SendStreamDataWithString(kCryptoStreamId, "foo", 0, !kFin, NULL);
// Check that ack is not bundled with outgoing data.
EXPECT_EQ(1u, writer_->frame_count());
EXPECT_FALSE(writer_->ack());
EXPECT_TRUE(connection_.GetAckAlarm()->IsSet());
// Check that ack is bundled with outgoing crypto data.
EXPECT_EQ(version() <= QUIC_VERSION_15 ? 2u : 3u, writer_->frame_count());
EXPECT_TRUE(writer_->ack());
EXPECT_FALSE(connection_.GetAckAlarm()->IsSet());
}

TEST_P(QuicConnectionTest, BundleAckWithDataOnIncomingAck) {
Expand Down
37 changes: 26 additions & 11 deletions net/quic/quic_framer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,10 +182,13 @@ QuicFramer::~QuicFramer() {}
size_t QuicFramer::GetMinStreamFrameSize(QuicVersion version,
QuicStreamId stream_id,
QuicStreamOffset offset,
bool last_frame_in_packet) {
bool last_frame_in_packet,
InFecGroup is_in_fec_group) {
bool no_stream_frame_length = last_frame_in_packet &&
is_in_fec_group == NOT_IN_FEC_GROUP;
return kQuicFrameTypeSize + GetStreamIdSize(stream_id) +
GetStreamOffsetSize(offset) +
(last_frame_in_packet ? 0 : kQuicStreamPayloadLengthSize);
(no_stream_frame_length ? 0 : kQuicStreamPayloadLengthSize);
}

// static
Expand Down Expand Up @@ -289,13 +292,15 @@ size_t QuicFramer::GetSerializedFrameLength(
size_t free_bytes,
bool first_frame,
bool last_frame,
InFecGroup is_in_fec_group,
QuicSequenceNumberLength sequence_number_length) {
if (frame.type == PADDING_FRAME) {
// PADDING implies end of packet.
return free_bytes;
}
size_t frame_len =
ComputeFrameLength(frame, last_frame, sequence_number_length);
ComputeFrameLength(frame, last_frame, is_in_fec_group,
sequence_number_length);
if (frame_len > free_bytes) {
// Only truncate the first frame in a packet, so if subsequent ones go
// over, stop including more frames.
Expand Down Expand Up @@ -335,6 +340,7 @@ SerializedPacket QuicFramer::BuildUnsizedDataPacket(
bool last_frame = i == frames.size() - 1;
const size_t frame_size = GetSerializedFrameLength(
frames[i], max_plaintext_size - packet_size, first_frame, last_frame,
header.is_in_fec_group,
header.public_header.sequence_number_length);
DCHECK(frame_size);
packet_size += frame_size;
Expand All @@ -357,8 +363,11 @@ SerializedPacket QuicFramer::BuildDataPacket(
for (size_t i = 0; i < frames.size(); ++i) {
const QuicFrame& frame = frames[i];

const bool last_frame_in_packet = i == (frames.size() - 1);
if (!AppendTypeByte(frame, last_frame_in_packet, &writer)) {
// Determine if we should write stream frame length in header.
const bool no_stream_frame_length =
(header.is_in_fec_group == NOT_IN_FEC_GROUP) &&
(i == frames.size() - 1);
if (!AppendTypeByte(frame, no_stream_frame_length, &writer)) {
LOG(DFATAL) << "AppendTypeByte failed";
return kNoPacket;
}
Expand All @@ -369,7 +378,7 @@ SerializedPacket QuicFramer::BuildDataPacket(
break;
case STREAM_FRAME:
if (!AppendStreamFrame(
*frame.stream_frame, last_frame_in_packet, &writer)) {
*frame.stream_frame, no_stream_frame_length, &writer)) {
LOG(DFATAL) << "AppendStreamFrame failed";
return kNoPacket;
}
Expand Down Expand Up @@ -1863,13 +1872,15 @@ size_t QuicFramer::GetAckFrameSize(
size_t QuicFramer::ComputeFrameLength(
const QuicFrame& frame,
bool last_frame_in_packet,
InFecGroup is_in_fec_group,
QuicSequenceNumberLength sequence_number_length) {
switch (frame.type) {
case STREAM_FRAME:
return GetMinStreamFrameSize(quic_version_,
frame.stream_frame->stream_id,
frame.stream_frame->offset,
last_frame_in_packet) +
last_frame_in_packet,
is_in_fec_group) +
frame.stream_frame->data.TotalBufferSize();
case ACK_FRAME: {
return GetAckFrameSize(*frame.ack_frame, sequence_number_length);
Expand Down Expand Up @@ -1941,7 +1952,7 @@ size_t QuicFramer::ComputeFrameLength(
}

bool QuicFramer::AppendTypeByte(const QuicFrame& frame,
bool last_frame_in_packet,
bool no_stream_frame_length,
QuicDataWriter* writer) {
uint8 type_byte = 0;
switch (frame.type) {
Expand All @@ -1954,7 +1965,7 @@ bool QuicFramer::AppendTypeByte(const QuicFrame& frame,

// Data Length bit.
type_byte <<= kQuicStreamDataLengthShift;
type_byte |= last_frame_in_packet ? 0 : kQuicStreamDataLengthMask;
type_byte |= no_stream_frame_length ? 0: kQuicStreamDataLengthMask;

// Offset 3 bits.
type_byte <<= kQuicStreamOffsetShift;
Expand Down Expand Up @@ -2019,21 +2030,25 @@ bool QuicFramer::AppendPacketSequenceNumber(

bool QuicFramer::AppendStreamFrame(
const QuicStreamFrame& frame,
bool last_frame_in_packet,
bool no_stream_frame_length,
QuicDataWriter* writer) {
if (!writer->WriteBytes(&frame.stream_id, GetStreamIdSize(frame.stream_id))) {
LOG(DFATAL) << "Writing stream id size failed.";
return false;
}
if (!writer->WriteBytes(&frame.offset, GetStreamOffsetSize(frame.offset))) {
LOG(DFATAL) << "Writing offset size failed.";
return false;
}
if (!last_frame_in_packet) {
if (!no_stream_frame_length) {
if (!writer->WriteUInt16(frame.data.TotalBufferSize())) {
LOG(DFATAL) << "Writing stream frame length failed";
return false;
}
}

if (!writer->WriteIOVector(frame.data)) {
LOG(DFATAL) << "Writing frame data failed.";
return false;
}
return true;
Expand Down
9 changes: 6 additions & 3 deletions net/quic/quic_framer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,8 @@ class NET_EXPORT_PRIVATE QuicFramer {
static size_t GetMinStreamFrameSize(QuicVersion version,
QuicStreamId stream_id,
QuicStreamOffset offset,
bool last_frame_in_packet);
bool last_frame_in_packet,
InFecGroup is_in_fec_group);
// Size in bytes of all ack frame fields without the missing packets.
static size_t GetMinAckFrameSize(
QuicVersion version,
Expand Down Expand Up @@ -278,8 +279,9 @@ class NET_EXPORT_PRIVATE QuicFramer {
size_t GetSerializedFrameLength(
const QuicFrame& frame,
size_t free_bytes,
bool first_frame,
bool last_frame,
bool first_frame_in_packet,
bool last_frame_in_packet,
InFecGroup is_in_fec_group,
QuicSequenceNumberLength sequence_number_length);

// Returns the associated data from the encrypted packet |encrypted| as a
Expand Down Expand Up @@ -432,6 +434,7 @@ class NET_EXPORT_PRIVATE QuicFramer {
// Computes the wire size in bytes of the payload of |frame|.
size_t ComputeFrameLength(const QuicFrame& frame,
bool last_frame_in_packet,
InFecGroup is_in_fec_group,
QuicSequenceNumberLength sequence_number_length);

static bool AppendPacketSequenceNumber(
Expand Down
56 changes: 56 additions & 0 deletions net/quic/quic_framer_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3899,6 +3899,62 @@ TEST_P(QuicFramerTest, BuildStreamFramePacket) {
AsChars(packet), arraysize(packet));
}

TEST_P(QuicFramerTest, BuildStreamFramePacketInFecGroup) {
QuicPacketHeader header;
header.public_header.connection_id = GG_UINT64_C(0xFEDCBA9876543210);
header.public_header.reset_flag = false;
header.public_header.version_flag = false;
header.fec_flag = false;
header.entropy_flag = true;
header.packet_sequence_number = GG_UINT64_C(0x77123456789ABC);
header.is_in_fec_group = IN_FEC_GROUP;
header.fec_group = GG_UINT64_C(0x77123456789ABC);

QuicStreamFrame stream_frame;
stream_frame.stream_id = 0x01020304;
stream_frame.fin = true;
stream_frame.offset = GG_UINT64_C(0xBA98FEDC32107654);
stream_frame.data = MakeIOVector("hello world!");

QuicFrames frames;
frames.push_back(QuicFrame(&stream_frame));
unsigned char packet[] = {
// public flags (8 byte connection_id)
0x3C,
// connection_id
0x10, 0x32, 0x54, 0x76,
0x98, 0xBA, 0xDC, 0xFE,
// packet sequence number
0xBC, 0x9A, 0x78, 0x56,
0x34, 0x12,
// private flags (entropy, is_in_fec_group)
0x03,
// FEC group
0x00,
// frame type (stream frame with fin and data length field)
0xFF,
// stream id
0x04, 0x03, 0x02, 0x01,
// offset
0x54, 0x76, 0x10, 0x32,
0xDC, 0xFE, 0x98, 0xBA,
// data length (since packet is in an FEC group)
0x0C, 0x00,
// data
'h', 'e', 'l', 'l',
'o', ' ', 'w', 'o',
'r', 'l', 'd', '!',
};

scoped_ptr<QuicPacket> data(
framer_.BuildUnsizedDataPacket(header, frames).packet);
ASSERT_TRUE(data != NULL);

test::CompareCharArraysWithHexError("constructed packet",
data->data(), data->length(),
AsChars(packet), arraysize(packet));
}

TEST_P(QuicFramerTest, BuildStreamFramePacketWithVersionFlag) {
QuicPacketHeader header;
header.public_header.connection_id = GG_UINT64_C(0xFEDCBA9876543210);
Expand Down
1 change: 1 addition & 0 deletions net/quic/quic_http_stream_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ class QuicHttpStreamTest : public ::testing::TestWithParam<QuicVersion> {
stream_.reset(use_closing_stream_ ?
new AutoClosingStream(session_->GetWeakPtr()) :
new QuicHttpStream(session_->GetWeakPtr()));
clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(20));
}

void SetRequest(const std::string& method,
Expand Down
1 change: 1 addition & 0 deletions net/quic/quic_network_transaction_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ class QuicNetworkTransactionTest
request_.method = "GET";
request_.url = GURL("http://www.google.com/");
request_.load_flags = 0;
clock_->AdvanceTime(QuicTime::Delta::FromMilliseconds(20));
}

virtual void SetUp() {
Expand Down
Loading

0 comments on commit 98a9d12

Please sign in to comment.