From e609bb94b0cf64a4373261854cfa45cfa87f6004 Mon Sep 17 00:00:00 2001 From: "sergeyu@chromium.org" Date: Thu, 18 Nov 2010 01:54:55 +0000 Subject: Use CompoundBuffer in MessageDecoder. TEST=Unittests BUG=None Review URL: http://codereview.chromium.org/5067001 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@66574 0039d316-1c4b-4281-b951-d872f2087c98 --- remoting/base/compound_buffer.cc | 43 +++++++++++++++++ remoting/base/compound_buffer.h | 4 ++ remoting/base/compound_buffer_unittest.cc | 37 +++++++++++++++ remoting/protocol/message_decoder.cc | 79 +++++++------------------------ remoting/protocol/message_decoder.h | 22 ++------- 5 files changed, 106 insertions(+), 79 deletions(-) (limited to 'remoting') diff --git a/remoting/base/compound_buffer.cc b/remoting/base/compound_buffer.cc index 9e8b8c3..2808f6d 100644 --- a/remoting/base/compound_buffer.cc +++ b/remoting/base/compound_buffer.cc @@ -88,6 +88,49 @@ void CompoundBuffer::PrependCopyOf(const char* data, int size) { Prepend(buffer, size); } +void CompoundBuffer::CropFront(int bytes) { + CHECK(!locked_); + + if (total_bytes_ <= bytes) { + Clear(); + return; + } + + total_bytes_ -= bytes; + while (chunks_.size() > 0 && chunks_.front().size <= bytes) { + bytes -= chunks_.front().size; + chunks_.pop_front(); + } + if (chunks_.size() > 0 && bytes > 0) { + chunks_.front().start += bytes; + chunks_.front().size -= bytes; + DCHECK_GT(chunks_.front().size, 0); + bytes = 0; + } + DCHECK_EQ(bytes, 0); +} + +void CompoundBuffer::CropBack(int bytes) { + CHECK(!locked_); + + if (total_bytes_ <= bytes) { + Clear(); + return; + } + + total_bytes_ -= bytes; + while (chunks_.size() > 0 && chunks_.back().size <= bytes) { + bytes -= chunks_.back().size; + chunks_.pop_back(); + } + if (chunks_.size() > 0 && bytes > 0) { + chunks_.back().size -= bytes; + DCHECK_GT(chunks_.back().size, 0); + bytes = 0; + } + DCHECK_EQ(bytes, 0); +} + void CompoundBuffer::Lock() { locked_ = true; } diff --git a/remoting/base/compound_buffer.h b/remoting/base/compound_buffer.h index 050182a..1b0a546 100644 --- a/remoting/base/compound_buffer.h +++ b/remoting/base/compound_buffer.h @@ -52,6 +52,10 @@ class CompoundBuffer { void AppendCopyOf(const char* data, int data_size); void PrependCopyOf(const char* data, int data_size); + // Drop |bytes| bytes from the beginning or the end of the buffer. + void CropFront(int bytes); + void CropBack(int bytes); + // Current size of the buffer. int total_bytes() const { return total_bytes_; } diff --git a/remoting/base/compound_buffer_unittest.cc b/remoting/base/compound_buffer_unittest.cc index 63ede5a..2b90bfe 100644 --- a/remoting/base/compound_buffer_unittest.cc +++ b/remoting/base/compound_buffer_unittest.cc @@ -24,6 +24,9 @@ const int kChunkSizes1[] = {1, 10, 20, -1}; // Chunk sizes used to test CopyFrom(). const int kCopySizes0[] = {10, 3, -1}; const int kCopySizes1[] = {20, -1}; + +const int kCropSizes[] = {1, -1}; + } // namespace class CompoundBufferTest : public testing::Test { @@ -52,6 +55,22 @@ class CompoundBufferTest : public testing::Test { EXPECT_TRUE(CompareData(copy, data_->data() + pos, size)); } + void TestCropFront(int pos, int size) { + CompoundBuffer cropped; + cropped.CopyFrom(target_, 0, target_.total_bytes()); + cropped.CropFront(pos); + EXPECT_TRUE(CompareData(cropped, data_->data() + pos, + target_.total_bytes() - pos)); + } + + void TestCropBack(int pos, int size) { + CompoundBuffer cropped; + cropped.CopyFrom(target_, 0, target_.total_bytes()); + cropped.CropBack(pos); + EXPECT_TRUE(CompareData(cropped, data_->data(), + target_.total_bytes() - pos)); + } + protected: virtual void SetUp() { data_ = new IOBuffer(kDataSize); @@ -210,6 +229,24 @@ TEST_F(CompoundBufferTest, PrependCopyOf) { EXPECT_TRUE(CompareData(target_, data_->data(), kDataSize)); } +TEST_F(CompoundBufferTest, CropFront) { + target_.Clear(); + IterateOverPieces(kChunkSizes1, NewCallback( + static_cast(this), &CompoundBufferTest::Append)); + IterateOverPieces(kCropSizes, NewCallback( + static_cast(this), + &CompoundBufferTest::TestCropFront)); +} + +TEST_F(CompoundBufferTest, CropBack) { + target_.Clear(); + IterateOverPieces(kChunkSizes1, NewCallback( + static_cast(this), &CompoundBufferTest::Append)); + IterateOverPieces(kCropSizes, NewCallback( + static_cast(this), + &CompoundBufferTest::TestCropBack)); +} + TEST_F(CompoundBufferTest, CopyFrom) { target_.Clear(); IterateOverPieces(kChunkSizes1, NewCallback( diff --git a/remoting/protocol/message_decoder.cc b/remoting/protocol/message_decoder.cc index 2e31bbc..cf44d45 100644 --- a/remoting/protocol/message_decoder.cc +++ b/remoting/protocol/message_decoder.cc @@ -13,8 +13,7 @@ namespace remoting { MessageDecoder::MessageDecoder() - : available_bytes_(0), - next_payload_(0), + : next_payload_(0), next_payload_known_(false) { } @@ -22,12 +21,10 @@ MessageDecoder::~MessageDecoder() {} void MessageDecoder::AddBuffer(scoped_refptr data, int data_size) { - buffer_list_.push_back(make_scoped_refptr( - new net::DrainableIOBuffer(data, data_size))); - available_bytes_ += data_size; + buffer_.Append(data, data_size); } -CompoundBuffer* MessageDecoder::CreateCompoundBufferFromData() { +bool MessageDecoder::GetNextMessageData(CompoundBuffer* message_buffer) { // Determine the payload size. If we already know it then skip this part. // We may not have enough data to determine the payload size so use a // utility function to find out. @@ -40,72 +37,30 @@ CompoundBuffer* MessageDecoder::CreateCompoundBufferFromData() { // If the next payload size is still not known or we don't have enough // data for parsing then exit. - if (!next_payload_known_ || available_bytes_ < next_payload_) - return NULL; - next_payload_known_ = false; - - // The following loop gather buffers in |buffer_list_| that sum up to - // |next_payload_| bytes. These buffers are added to |stream|. - - // Create a CompoundBuffer for parsing. - CompoundBuffer* result = new CompoundBuffer(); - while (next_payload_ > 0 && !buffer_list_.empty()) { - scoped_refptr buffer = buffer_list_.front(); - int read_bytes = std::min(buffer->BytesRemaining(), next_payload_); - - // This will reference the same base pointer but maintain it's own - // version of data pointer. - result->Append(buffer, read_bytes); - - // Adjust counters. - buffer->DidConsume(read_bytes); - next_payload_ -= read_bytes; - available_bytes_ -= read_bytes; + if (!next_payload_known_ || buffer_.total_bytes() < next_payload_) + return false; - // If the front buffer is fully read then remove it from the queue. - if (!buffer->BytesRemaining()) - buffer_list_.pop_front(); - } - DCHECK_EQ(0, next_payload_); - DCHECK_LE(0, available_bytes_); - result->Lock(); - return result; -} + message_buffer->CopyFrom(buffer_, 0, next_payload_); + message_buffer->Lock(); + buffer_.CropFront(next_payload_); + next_payload_known_ = false; -static int GetHeaderSize(const std::string& header) { - return header.length(); + return true; } bool MessageDecoder::GetPayloadSize(int* size) { // The header has a size of 4 bytes. const int kHeaderSize = sizeof(int32); - if (available_bytes_ < kHeaderSize) + if (buffer_.total_bytes() < kHeaderSize) return false; - std::string header; - while (GetHeaderSize(header) < kHeaderSize && !buffer_list_.empty()) { - scoped_refptr buffer = buffer_list_.front(); - - // Find out how many bytes we need and how many bytes are available in this - // buffer. - int needed_bytes = kHeaderSize - GetHeaderSize(header); - int available_bytes = buffer->BytesRemaining(); - - // Then append the required bytes into the header and advance the last - // read position. - int read_bytes = std::min(needed_bytes, available_bytes); - header.append(buffer->data(), read_bytes); - buffer->DidConsume(read_bytes); - available_bytes_ -= read_bytes; - - // If the buffer is depleted then remove it from the queue. - if (!buffer->BytesRemaining()) { - buffer_list_.pop_front(); - } - } - - *size = talk_base::GetBE32(header.c_str()); + CompoundBuffer header_buffer; + char header[kHeaderSize]; + header_buffer.CopyFrom(buffer_, 0, kHeaderSize); + header_buffer.CopyTo(header, kHeaderSize); + *size = talk_base::GetBE32(header); + buffer_.CropFront(kHeaderSize); return true; } diff --git a/remoting/protocol/message_decoder.h b/remoting/protocol/message_decoder.h index ea7acee..1978dc2 100644 --- a/remoting/protocol/message_decoder.h +++ b/remoting/protocol/message_decoder.h @@ -14,11 +14,6 @@ #include "remoting/base/compound_buffer.h" #include "third_party/protobuf/src/google/protobuf/message_lite.h" -namespace net { -class DrainableIOBuffer; -class IOBuffer; -} // namespace net - namespace remoting { class ChromotingClientMessage; @@ -66,18 +61,14 @@ class MessageDecoder { } private: - // TODO(sergeyu): It might be more efficient to memcopy data to one big buffer - // instead of storing chunks in dqueue. - typedef std::deque > BufferList; - // Parse one message from |buffer_list_|. Return true if sucessful. template bool ParseOneMessage(MessageType** message) { - scoped_ptr buffer(CreateCompoundBufferFromData()); - if (!buffer.get()) + CompoundBuffer buffer; + if (!GetNextMessageData(&buffer)) return false; - CompoundBufferInputStream stream(buffer.get()); + CompoundBufferInputStream stream(&buffer); *message = new MessageType(); bool ret = (*message)->ParseFromZeroCopyStream(&stream); if (!ret) @@ -87,17 +78,14 @@ class MessageDecoder { void AddBuffer(scoped_refptr data, int data_size); - CompoundBuffer* CreateCompoundBufferFromData(); + bool GetNextMessageData(CompoundBuffer* message_buffer); // Retrieves the read payload size of the current protocol buffer via |size|. // Returns false and leaves |size| unmodified, if we do not have enough data // to retrieve the current size. bool GetPayloadSize(int* size); - BufferList buffer_list_; - - // The number of bytes in |buffer_list_| not consumed. - int available_bytes_; + CompoundBuffer buffer_; // |next_payload_| stores the size of the next payload if known. // |next_payload_known_| is true if the size of the next payload is known. -- cgit v1.1