Commit 2ae71b61 authored by Dan Zhang's avatar Dan Zhang Committed by Commit Bot

Make sure QuicStreamSequencerBuffer only use 2048 blocks at most.

The old way of initialize QuicStreamSequencerBuffer::block_count_ may
assign it to 2049 instead of 2048 (16MB / 8KB = 2k). This cl fix this bug and also
make the condition check of a QUIC_BUG to check this value correctly.

R=rch@chromium.org

Merge internal change: 164489401, 174343515

Bug: 
Change-Id: Iaadc4084c470277d0f0882d75b4a61539c780aad
Reviewed-on: https://chromium-review.googlesource.com/793977Reviewed-by: default avatarRyan Hamilton <rch@chromium.org>
Commit-Queue: Dan Zhang <danzh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519814}
parent 0ac9c0ba
...@@ -209,3 +209,9 @@ QUIC_FLAG(bool, FLAGS_quic_reloadable_flag_quic_truncate_long_details, true) ...@@ -209,3 +209,9 @@ QUIC_FLAG(bool, FLAGS_quic_reloadable_flag_quic_truncate_long_details, true)
QUIC_FLAG(bool, QUIC_FLAG(bool,
FLAGS_quic_reloadable_flag_quic_allow_multiple_acks_for_data2, FLAGS_quic_reloadable_flag_quic_allow_multiple_acks_for_data2,
false) false)
// If true, calculate stream sequencer buffer block count in a way that
// guaranteed to be 2048.
QUIC_FLAG(bool,
FLAGS_quic_reloadable_flag_quic_fix_sequencer_buffer_block_count2,
false)
...@@ -7,6 +7,7 @@ ...@@ -7,6 +7,7 @@
#include "base/format_macros.h" #include "base/format_macros.h"
#include "net/quic/core/quic_constants.h" #include "net/quic/core/quic_constants.h"
#include "net/quic/platform/api/quic_bug_tracker.h" #include "net/quic/platform/api/quic_bug_tracker.h"
#include "net/quic/platform/api/quic_flag_utils.h"
#include "net/quic/platform/api/quic_flags.h" #include "net/quic/platform/api/quic_flags.h"
#include "net/quic/platform/api/quic_logging.h" #include "net/quic/platform/api/quic_logging.h"
#include "net/quic/platform/api/quic_str_cat.h" #include "net/quic/platform/api/quic_str_cat.h"
...@@ -16,6 +17,18 @@ using std::string; ...@@ -16,6 +17,18 @@ using std::string;
namespace net { namespace net {
namespace { namespace {
size_t CalculateBlockCount(size_t max_capacity_bytes) {
if (FLAGS_quic_reloadable_flag_quic_fix_sequencer_buffer_block_count2) {
QUIC_FLAG_COUNT(
quic_reloadable_flag_quic_fix_sequencer_buffer_block_count2);
return (max_capacity_bytes + QuicStreamSequencerBuffer::kBlockSizeBytes -
1) /
QuicStreamSequencerBuffer::kBlockSizeBytes;
}
return ceil(static_cast<double>(max_capacity_bytes) /
QuicStreamSequencerBuffer::kBlockSizeBytes);
}
// Upper limit of how many gaps allowed in buffer, which ensures a reasonable // Upper limit of how many gaps allowed in buffer, which ensures a reasonable
// number of iterations needed to find the right gap to fill when a frame // number of iterations needed to find the right gap to fill when a frame
// arrives. // arrives.
...@@ -36,8 +49,7 @@ QuicStreamSequencerBuffer::FrameInfo::FrameInfo(size_t length, ...@@ -36,8 +49,7 @@ QuicStreamSequencerBuffer::FrameInfo::FrameInfo(size_t length,
QuicStreamSequencerBuffer::QuicStreamSequencerBuffer(size_t max_capacity_bytes) QuicStreamSequencerBuffer::QuicStreamSequencerBuffer(size_t max_capacity_bytes)
: max_buffer_capacity_bytes_(max_capacity_bytes), : max_buffer_capacity_bytes_(max_capacity_bytes),
blocks_count_( blocks_count_(CalculateBlockCount(max_capacity_bytes)),
ceil(static_cast<double>(max_capacity_bytes) / kBlockSizeBytes)),
total_bytes_read_(0), total_bytes_read_(0),
blocks_(nullptr), blocks_(nullptr),
destruction_indicator_(123456) { destruction_indicator_(123456) {
......
...@@ -83,6 +83,17 @@ class QuicStreamSequencerBufferTest : public testing::Test { ...@@ -83,6 +83,17 @@ class QuicStreamSequencerBufferTest : public testing::Test {
string error_details_; string error_details_;
}; };
TEST_F(QuicStreamSequencerBufferTest, InitializeWithMaxRecvWindowSize) {
if (!FLAGS_quic_reloadable_flag_quic_fix_sequencer_buffer_block_count2) {
return;
}
ResetMaxCapacityBytes(16 * 1024 * 1024); // 16MB
EXPECT_EQ(2 * 1024u, // 16MB / 8KB = 2K
helper_->block_count());
EXPECT_EQ(max_capacity_bytes_, helper_->max_buffer_capacity());
EXPECT_TRUE(helper_->CheckInitialState());
}
TEST_F(QuicStreamSequencerBufferTest, InitializationWithDifferentSizes) { TEST_F(QuicStreamSequencerBufferTest, InitializationWithDifferentSizes) {
const size_t kCapacity = 2 * QuicStreamSequencerBuffer::kBlockSizeBytes; const size_t kCapacity = 2 * QuicStreamSequencerBuffer::kBlockSizeBytes;
ResetMaxCapacityBytes(kCapacity); ResetMaxCapacityBytes(kCapacity);
......
...@@ -139,5 +139,8 @@ bool QuicStreamSequencerBufferPeer::IsBufferAllocated() { ...@@ -139,5 +139,8 @@ bool QuicStreamSequencerBufferPeer::IsBufferAllocated() {
return buffer_->blocks_ != nullptr; return buffer_->blocks_ != nullptr;
} }
size_t QuicStreamSequencerBufferPeer::block_count() {
return buffer_->blocks_count_;
}
} // namespace test } // namespace test
} // namespace net } // namespace net
...@@ -51,6 +51,8 @@ class QuicStreamSequencerBufferPeer { ...@@ -51,6 +51,8 @@ class QuicStreamSequencerBufferPeer {
bool IsBufferAllocated(); bool IsBufferAllocated();
size_t block_count();
private: private:
QuicStreamSequencerBuffer* buffer_; QuicStreamSequencerBuffer* buffer_;
DISALLOW_COPY_AND_ASSIGN(QuicStreamSequencerBufferPeer); DISALLOW_COPY_AND_ASSIGN(QuicStreamSequencerBufferPeer);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment