This change simplifies SpdyHeaderBlock and SpdyHeaderStorage, now that the underlying arena implementation does not allocate memory upon construction.
gfe-relnote: Code simplifications to classes in //third_party/spdy, no functional change; not protected.
PiperOrigin-RevId: 284805587
Change-Id: I3b0cbdda821c1f2ea89b9f880d76a3132490b196
diff --git a/spdy/core/spdy_header_block.cc b/spdy/core/spdy_header_block.cc
index 9ccc627..609f6d7 100644
--- a/spdy/core/spdy_header_block.cc
+++ b/spdy/core/spdy_header_block.cc
@@ -12,7 +12,6 @@
#include "net/third_party/quiche/src/spdy/platform/api/spdy_estimate_memory_usage.h"
#include "net/third_party/quiche/src/spdy/platform/api/spdy_logging.h"
#include "net/third_party/quiche/src/spdy/platform/api/spdy_string_utils.h"
-#include "net/third_party/quiche/src/spdy/platform/api/spdy_unsafe_arena.h"
namespace spdy {
namespace {
@@ -65,6 +64,10 @@
return *this;
}
+void SpdyHeaderBlock::HeaderValue::set_storage(SpdyHeaderStorage* storage) {
+ storage_ = storage;
+}
+
SpdyHeaderBlock::HeaderValue::~HeaderValue() = default;
SpdyStringPiece SpdyHeaderBlock::HeaderValue::ConsolidatedValue() const {
@@ -132,14 +135,14 @@
// can reclaim the memory used by the key. This makes lookup-only access to
// SpdyHeaderBlock through operator[] memory-neutral.
if (valid_ && lookup_result_ == block_->map_.end()) {
- block_->GetStorage()->Rewind(key_);
+ block_->storage_.Rewind(key_);
}
}
SpdyHeaderBlock::ValueProxy& SpdyHeaderBlock::ValueProxy::operator=(
const SpdyStringPiece value) {
*spdy_header_block_value_size_ += value.size();
- SpdyHeaderStorage* storage = block_->GetStorage();
+ SpdyHeaderStorage* storage = &block_->storage_;
if (lookup_result_ == block_->map_.end()) {
SPDY_DVLOG(1) << "Inserting: (" << key_ << ", " << value << ")";
lookup_result_ =
@@ -168,7 +171,10 @@
SpdyHeaderBlock::SpdyHeaderBlock(SpdyHeaderBlock&& other)
: map_(kInitialMapBuckets) {
map_.swap(other.map_);
- storage_.swap(other.storage_);
+ storage_ = std::move(other.storage_);
+ for (auto& p : map_) {
+ p.second.set_storage(&storage_);
+ }
key_size_ = other.key_size_;
value_size_ = other.value_size_;
}
@@ -177,7 +183,10 @@
SpdyHeaderBlock& SpdyHeaderBlock::operator=(SpdyHeaderBlock&& other) {
map_.swap(other.map_);
- storage_.swap(other.storage_);
+ storage_ = std::move(other.storage_);
+ for (auto& p : map_) {
+ p.second.set_storage(&storage_);
+ }
key_size_ = other.key_size_;
value_size_ = other.value_size_;
return *this;
@@ -226,7 +235,7 @@
key_size_ = 0;
value_size_ = 0;
map_.clear();
- storage_.reset();
+ storage_.Clear();
}
void SpdyHeaderBlock::insert(const SpdyHeaderBlock::value_type& value) {
@@ -242,9 +251,8 @@
SPDY_DVLOG(1) << "Updating key: " << iter->first
<< " with value: " << value.second;
value_size_ -= iter->second.SizeEstimate();
- SpdyHeaderStorage* storage = GetStorage();
iter->second =
- HeaderValue(storage, iter->first, storage->Write(value.second));
+ HeaderValue(&storage_, iter->first, storage_.Write(value.second));
}
}
@@ -280,7 +288,7 @@
SPDY_DVLOG(1) << "Updating key: " << iter->first
<< "; appending value: " << value;
value_size_ += SeparatorForKey(key).size();
- iter->second.Append(GetStorage()->Write(value));
+ iter->second.Append(storage_.Write(value));
}
size_t SpdyHeaderBlock::EstimateMemoryUsage() const {
@@ -292,29 +300,17 @@
void SpdyHeaderBlock::AppendHeader(const SpdyStringPiece key,
const SpdyStringPiece value) {
auto backed_key = WriteKey(key);
- SpdyHeaderStorage* storage = GetStorage();
map_.emplace(std::make_pair(
- backed_key, HeaderValue(storage, backed_key, storage->Write(value))));
-}
-
-SpdyHeaderStorage* SpdyHeaderBlock::GetStorage() {
- if (storage_ == nullptr) {
- storage_ = std::make_unique<SpdyHeaderStorage>();
- }
- return storage_.get();
+ backed_key, HeaderValue(&storage_, backed_key, storage_.Write(value))));
}
SpdyStringPiece SpdyHeaderBlock::WriteKey(const SpdyStringPiece key) {
key_size_ += key.size();
- return GetStorage()->Write(key);
+ return storage_.Write(key);
}
size_t SpdyHeaderBlock::bytes_allocated() const {
- if (storage_ == nullptr) {
- return 0;
- } else {
- return storage_->bytes_allocated();
- }
+ return storage_.bytes_allocated();
}
} // namespace spdy
diff --git a/spdy/core/spdy_header_block.h b/spdy/core/spdy_header_block.h
index ff5f4c9..bd575f4 100644
--- a/spdy/core/spdy_header_block.h
+++ b/spdy/core/spdy_header_block.h
@@ -8,7 +8,6 @@
#include <stddef.h>
#include <list>
-#include <memory>
#include <string>
#include <utility>
#include <vector>
@@ -51,6 +50,8 @@
HeaderValue(HeaderValue&& other);
HeaderValue& operator=(HeaderValue&& other);
+ void set_storage(SpdyHeaderStorage* storage);
+
// Copies are not.
HeaderValue(const HeaderValue& other) = delete;
HeaderValue& operator=(const HeaderValue& other) = delete;
@@ -225,14 +226,12 @@
friend class test::SpdyHeaderBlockPeer;
void AppendHeader(const SpdyStringPiece key, const SpdyStringPiece value);
- SpdyHeaderStorage* GetStorage();
SpdyStringPiece WriteKey(const SpdyStringPiece key);
size_t bytes_allocated() const;
- // SpdyStringPieces held by |map_| point to memory owned by |*storage_|.
- // |storage_| might be nullptr as long as |map_| is empty.
+ // SpdyStringPieces held by |map_| point to memory owned by |storage_|.
MapType map_;
- std::unique_ptr<SpdyHeaderStorage> storage_;
+ SpdyHeaderStorage storage_;
size_t key_size_ = 0;
size_t value_size_ = 0;
diff --git a/spdy/core/spdy_header_block_test.cc b/spdy/core/spdy_header_block_test.cc
index a744a3b..774854b 100644
--- a/spdy/core/spdy_header_block_test.cc
+++ b/spdy/core/spdy_header_block_test.cc
@@ -118,6 +118,13 @@
EXPECT_NE(block1, block2);
}
+SpdyHeaderBlock ReturnTestHeaderBlock() {
+ SpdyHeaderBlock block;
+ block["foo"] = "bar";
+ block.insert(std::make_pair("foo2", "baz"));
+ return block;
+}
+
// Test that certain methods do not crash on moved-from instances.
TEST(SpdyHeaderBlockTest, MovedFromIsValid) {
SpdyHeaderBlock block1;
@@ -139,6 +146,11 @@
block1["foo"] = "bar";
EXPECT_THAT(block1, ElementsAre(Pair("foo", "bar")));
+
+ SpdyHeaderBlock block5 = ReturnTestHeaderBlock();
+ block5.AppendValueOrAddHeader("foo", "bar2");
+ EXPECT_THAT(block5, ElementsAre(Pair("foo", std::string("bar\0bar2", 8)),
+ Pair("foo2", "baz")));
}
// This test verifies that headers can be appended to no matter how they were
diff --git a/spdy/core/spdy_header_storage.h b/spdy/core/spdy_header_storage.h
index fd31555..fc559ec 100644
--- a/spdy/core/spdy_header_storage.h
+++ b/spdy/core/spdy_header_storage.h
@@ -23,6 +23,9 @@
SpdyHeaderStorage(const SpdyHeaderStorage&) = delete;
SpdyHeaderStorage& operator=(const SpdyHeaderStorage&) = delete;
+ SpdyHeaderStorage(SpdyHeaderStorage&& other) = default;
+ SpdyHeaderStorage& operator=(SpdyHeaderStorage&& other) = default;
+
SpdyStringPiece Write(SpdyStringPiece s);
// If |s| points to the most recent allocation from arena_, the arena will
