Modify SpdyHeaderBlock's internals to consolidate header values only on first access.

This CL lands server changes 143452565 by birenroy and 143815813 by bnc.

BUG=488484

Review-Url: https://codereview.chromium.org/2611173004
Cr-Commit-Position: refs/heads/master@{#442924}

net/base/arena.cc

@@ -68,6 +68,7 @@ void UnsafeArena::Free(char* data, size_t size) {
 
 void UnsafeArena::Reset() {
   blocks_.clear();
+  status_.bytes_allocated_ = 0;
 }
 
 void UnsafeArena::Reserve(size_t additional_space) {
@@ -83,6 +84,7 @@ void UnsafeArena::Reserve(size_t additional_space) {
 
 void UnsafeArena::AllocBlock(size_t size) {
   blocks_.push_back(Block(size));
+  status_.bytes_allocated_ += size;
 }
 
 UnsafeArena::Block::Block(size_t s) : data(new char[s]), size(s), used(0) {}

net/base/arena.h

@@ -16,6 +16,16 @@ namespace net {
 // Not thread-safe.
 class NET_EXPORT_PRIVATE UnsafeArena {
  public:
+  class Status {
+   private:
+    friend class UnsafeArena;
+    size_t bytes_allocated_;
+
+   public:
+    Status() : bytes_allocated_(0) {}
+    size_t bytes_allocated() const { return bytes_allocated_; }
+  };
+
   // Blocks allocated by this arena will be at least |block_size| bytes.
   explicit UnsafeArena(size_t block_size);
   ~UnsafeArena();
@@ -39,6 +49,8 @@ class NET_EXPORT_PRIVATE UnsafeArena {
 
   void Reset();
 
+  Status status() const { return status_; }
+
  private:
   struct Block {
     std::unique_ptr<char[]> data;
@@ -57,6 +69,7 @@ class NET_EXPORT_PRIVATE UnsafeArena {
 
   size_t block_size_;
   std::vector<Block> blocks_;
+  Status status_;
 };
 
 }  // namespace net

net/spdy/spdy_header_block.cc

@@ -19,6 +19,7 @@
 using base::StringPiece;
 using std::dec;
 using std::hex;
+using std::make_pair;
 using std::max;
 using std::min;
 using std::string;
@@ -30,6 +31,16 @@ namespace {
 const size_t kDefaultStorageBlockSize = 2048;
 
 const char kCookieKey[] = "cookie";
+const char kNullSeparator = 0;
+
+StringPiece SeparatorForKey(StringPiece key) {
+  if (key == kCookieKey) {
+    static StringPiece cookie_separator = "; ";
+    return cookie_separator;
+  } else {
+    return StringPiece(&kNullSeparator, 1);
+  }
+}
 
 }  // namespace
 
@@ -50,24 +61,6 @@ class SpdyHeaderBlock::Storage {
     return StringPiece(arena_.Memdup(s.data(), s.size()), s.size());
   }
 
-  // Given value, a string already in the arena, perform a realloc and append
-  // separator and more to the end of the value's new location. If value is the
-  // most recently added string (via Write), then UnsafeArena will not copy the
-  // existing value but instead will increase the space reserved for value.
-  StringPiece Realloc(StringPiece value,
-                      StringPiece separator,
-                      StringPiece more) {
-    size_t total_length = value.size() + separator.size() + more.size();
-    char* ptr = const_cast<char*>(value.data());
-    ptr = arena_.Realloc(ptr, value.size(), total_length);
-    StringPiece result(ptr, total_length);
-    ptr += value.size();
-    memcpy(ptr, separator.data(), separator.size());
-    ptr += separator.size();
-    memcpy(ptr, more.data(), more.size());
-    return result;
-  }
-
   // If |s| points to the most recent allocation from arena_, the arena will
   // reclaim the memory. Otherwise, this method is a no-op.
   void Rewind(const StringPiece s) {
@@ -76,10 +69,77 @@ class SpdyHeaderBlock::Storage {
 
   void Clear() { arena_.Reset(); }
 
+  // Given a list of fragments and a separator, writes the fragments joined by
+  // the separator to a contiguous region of memory. Returns a StringPiece
+  // pointing to the region of memory.
+  StringPiece WriteFragments(const std::vector<StringPiece>& fragments,
+                             StringPiece separator) {
+    if (fragments.empty()) {
+      return StringPiece();
+    }
+    size_t total_size = separator.size() * (fragments.size() - 1);
+    for (const auto fragment : fragments) {
+      total_size += fragment.size();
+    }
+    char* dst = arena_.Alloc(total_size);
+    size_t written = Join(dst, fragments, separator);
+    DCHECK_EQ(written, total_size);
+    return StringPiece(dst, total_size);
+  }
+
+  size_t bytes_allocated() const { return arena_.status().bytes_allocated(); }
+
  private:
   UnsafeArena arena_;
 };
 
+SpdyHeaderBlock::HeaderValue::HeaderValue(Storage* storage,
+                                          StringPiece key,
+                                          StringPiece initial_value)
+    : storage_(storage), fragments_({initial_value}), pair_({key, {}}) {}
+
+SpdyHeaderBlock::HeaderValue::HeaderValue(HeaderValue&& other)
+    : storage_(other.storage_),
+      fragments_(std::move(other.fragments_)),
+      pair_(std::move(other.pair_)) {}
+
+SpdyHeaderBlock::HeaderValue& SpdyHeaderBlock::HeaderValue::operator=(
+    HeaderValue&& other) {
+  storage_ = other.storage_;
+  fragments_ = std::move(other.fragments_);
+  pair_ = std::move(other.pair_);
+  return *this;
+}
+
+SpdyHeaderBlock::HeaderValue::~HeaderValue() {}
+
+StringPiece SpdyHeaderBlock::HeaderValue::ConsolidatedValue() const {
+  if (fragments_.empty()) {
+    return StringPiece();
+  }
+  if (fragments_.size() > 1) {
+    fragments_ = {
+        storage_->WriteFragments(fragments_, SeparatorForKey(pair_.first))};
+  }
+  return fragments_[0];
+}
+
+void SpdyHeaderBlock::HeaderValue::Append(StringPiece fragment) {
+  fragments_.push_back(fragment);
+}
+
+const std::pair<StringPiece, StringPiece>&
+SpdyHeaderBlock::HeaderValue::as_pair() const {
+  pair_.second = ConsolidatedValue();
+  return pair_;
+}
+
+SpdyHeaderBlock::iterator::iterator(MapType::const_iterator it) : it_(it) {}
+
+SpdyHeaderBlock::iterator::iterator(const iterator& other) : it_(other.it_) {}
+
+SpdyHeaderBlock::iterator::~iterator() {}
+
 SpdyHeaderBlock::ValueProxy::ValueProxy(
     SpdyHeaderBlock::MapType* block,
     SpdyHeaderBlock::Storage* storage,
@@ -89,8 +149,7 @@ SpdyHeaderBlock::ValueProxy::ValueProxy(
       storage_(storage),
       lookup_result_(lookup_result),
       key_(key),
-      valid_(true) {
-}
+      valid_(true) {}
 
 SpdyHeaderBlock::ValueProxy::ValueProxy(ValueProxy&& other)
     : block_(other.block_),
@@ -127,10 +186,14 @@ SpdyHeaderBlock::ValueProxy& SpdyHeaderBlock::ValueProxy::operator=(
   if (lookup_result_ == block_->end()) {
     DVLOG(1) << "Inserting: (" << key_ << ", " << value << ")";
     lookup_result_ =
-        block_->insert(std::make_pair(key_, storage_->Write(value))).first;
+        block_
+            ->emplace(make_pair(
+                key_, HeaderValue(storage_, key_, storage_->Write(value))))
+            .first;
   } else {
     DVLOG(1) << "Updating key: " << key_ << " with value: " << value;
-    lookup_result_->second = storage_->Write(value);
+    lookup_result_->second =
+        HeaderValue(storage_, key_, storage_->Write(value));
   }
   return *this;
 }
@@ -139,7 +202,7 @@ string SpdyHeaderBlock::ValueProxy::as_string() const {
   if (lookup_result_ == block_->end()) {
     return "";
   } else {
-    return lookup_result_->second.as_string();
+    return lookup_result_->second.value().as_string();
   }
 }
 
@@ -160,8 +223,8 @@ SpdyHeaderBlock& SpdyHeaderBlock::operator=(SpdyHeaderBlock&& other) {
 
 SpdyHeaderBlock SpdyHeaderBlock::Clone() const {
   SpdyHeaderBlock copy;
-  for (auto iter : *this) {
-    copy.AppendHeader(iter.first, iter.second);
+  for (const auto& p : *this) {
+    copy.AppendHeader(p.first, p.second);
   }
   return copy;
 }
@@ -181,7 +244,7 @@ string SpdyHeaderBlock::DebugString() const {
   string output = "\n{\n";
   for (auto it = begin(); it != end(); ++it) {
     output +=
-        "  " + it->first.as_string() + ":" + it->second.as_string() + "\n";
+        "  " + it->first.as_string() + " " + it->second.as_string() + "\n";
   }
   output.append("}\n");
   return output;
@@ -192,8 +255,7 @@ void SpdyHeaderBlock::clear() {
   storage_.reset();
 }
 
-void SpdyHeaderBlock::insert(
-    const SpdyHeaderBlock::MapType::value_type& value) {
+void SpdyHeaderBlock::insert(const SpdyHeaderBlock::value_type& value) {
   // TODO(birenroy): Write new value in place of old value, if it fits.
   auto iter = block_.find(value.first);
   if (iter == block_.end()) {
@@ -202,7 +264,9 @@ void SpdyHeaderBlock::insert(
   } else {
     DVLOG(1) << "Updating key: " << iter->first
              << " with value: " << value.second;
-    iter->second = GetStorage()->Write(value.second);
+    auto storage = GetStorage();
+    iter->second =
+        HeaderValue(storage, iter->first, storage->Write(value.second));
   }
 }
 
@@ -232,16 +296,15 @@ void SpdyHeaderBlock::AppendValueOrAddHeader(const StringPiece key,
     return;
   }
   DVLOG(1) << "Updating key: " << iter->first << "; appending value: " << value;
-  StringPiece separator("", 1);
-  if (key == kCookieKey) {
-    separator = "; ";
-  }
-  iter->second = GetStorage()->Realloc(iter->second, separator, value);
+  iter->second.Append(GetStorage()->Write(value));
 }
 
 void SpdyHeaderBlock::AppendHeader(const StringPiece key,
                                    const StringPiece value) {
-  block_.emplace(GetStorage()->Write(key), GetStorage()->Write(value));
+  auto storage = GetStorage();
+  auto backed_key = storage->Write(key);
+  block_.emplace(make_pair(
+      backed_key, HeaderValue(storage, backed_key, storage->Write(value))));
 }
 
 SpdyHeaderBlock::Storage* SpdyHeaderBlock::GetStorage() {
@@ -293,4 +356,31 @@ bool SpdyHeaderBlockFromNetLogParam(
   return true;
 }
 
+size_t SpdyHeaderBlock::bytes_allocated() const {
+  if (storage_ == nullptr) {
+    return 0;
+  } else {
+    return storage_->bytes_allocated();
+  }
+}
+
+size_t Join(char* dst,
+            const std::vector<StringPiece>& fragments,
+            StringPiece separator) {
+  if (fragments.empty()) {
+    return 0;
+  }
+  auto original_dst = dst;
+  auto it = fragments.begin();
+  memcpy(dst, it->data(), it->size());
+  dst += it->size();
+  for (++it; it != fragments.end(); ++it) {
+    memcpy(dst, separator.data(), separator.size());
+    dst += separator.size();
+    memcpy(dst, it->data(), it->size());
+    dst += it->size();
+  }
+  return dst - original_dst;
+}
+
 }  // namespace net

net/spdy/spdy_header_block.h

@@ -11,6 +11,7 @@
 #include <map>
 #include <memory>
 #include <string>
+#include <vector>
 
 #include "base/macros.h"
 #include "base/strings/string_piece.h"
@@ -27,6 +28,7 @@ namespace net {
 class NetLogCaptureMode;
 
 namespace test {
+class SpdyHeaderBlockPeer;
 class ValueProxyPeer;
 }
 
@@ -43,16 +45,93 @@ class ValueProxyPeer;
 // It's expected that keys are rarely deleted from a SpdyHeaderBlock.
 class NET_EXPORT SpdyHeaderBlock {
  private:
-  using MapType = linked_hash_map<base::StringPiece,
-                                  base::StringPiece,
-                                  base::StringPieceHash>;
   class Storage;
 
+  // Stores a list of value fragments that can be joined later with a
+  // key-dependent separator.
+  class NET_EXPORT HeaderValue {
+   public:
+    HeaderValue(Storage* storage,
+                base::StringPiece key,
+                base::StringPiece initial_value);
+
+    // Moves are allowed.
+    HeaderValue(HeaderValue&& other);
+    HeaderValue& operator=(HeaderValue&& other);
+
+    // Copies are not.
+    HeaderValue(const HeaderValue& other) = delete;
+    HeaderValue& operator=(const HeaderValue& other) = delete;
+
+    ~HeaderValue();
+
+    // Consumes at most |fragment.size()| bytes of memory.
+    void Append(base::StringPiece fragment);
+
+    base::StringPiece value() const { return as_pair().second; }
+    const std::pair<base::StringPiece, base::StringPiece>& as_pair() const;
+
+   private:
+    // May allocate a large contiguous region of memory to hold the concatenated
+    // fragments and separators.
+    base::StringPiece ConsolidatedValue() const;
+
+    mutable Storage* storage_;
+    mutable std::vector<base::StringPiece> fragments_;
+    // The first element is the key; the second is the consolidated value.
+    mutable std::pair<base::StringPiece, base::StringPiece> pair_;
+  };
+
+  typedef linked_hash_map<base::StringPiece, HeaderValue, base::StringPieceHash>
+      MapType;
+
  public:
-  using iterator = MapType::iterator;
-  using const_iterator = MapType::const_iterator;
-  using value_type = MapType::value_type;
-  using reverse_iterator = MapType::reverse_iterator;
+  typedef std::pair<base::StringPiece, base::StringPiece> value_type;
+
+  // Provides iteration over a sequence of std::pair<StringPiece, StringPiece>,
+  // even though the underlying MapType::value_type is different. Dereferencing
+  // the iterator will result in memory allocation for multi-value headers.
+  class NET_EXPORT iterator {
+   public:
+    // The following type definitions fulfill the requirements for iterator
+    // implementations.
+    typedef std::pair<base::StringPiece, base::StringPiece> value_type;
+    typedef value_type& reference;
+    typedef value_type* pointer;
+    typedef std::forward_iterator_tag iterator_category;
+    typedef MapType::iterator::difference_type difference_type;
+
+    // In practice, this iterator only offers access to const value_type.
+    typedef const value_type& const_reference;
+    typedef const value_type* const_pointer;
+
+    explicit iterator(MapType::const_iterator it);
+    iterator(const iterator& other);
+    ~iterator();
+
+    // This will result in memory allocation if the value consists of multiple
+    // fragments.
+    const_reference operator*() const { return it_->second.as_pair(); }
+
+    const_pointer operator->() const { return &(this->operator*()); }
+    bool operator==(const iterator& it) const { return it_ == it.it_; }
+    bool operator!=(const iterator& it) const { return !(*this == it); }
+
+    iterator& operator++() {
+      it_++;
+      return *this;
+    }
+
+    iterator operator++(int) {
+      auto ret = *this;
+      this->operator++();
+      return ret;
+    }
+
+   private:
+    MapType::const_iterator it_;
+  };
+  typedef iterator const_iterator;
 
   class ValueProxy;
 
@@ -72,16 +151,16 @@ class NET_EXPORT SpdyHeaderBlock {
   // keys and values.
   std::string DebugString() const;
 
-  // These methods delegate to our MapType member.
-  iterator begin() { return block_.begin(); }
-  iterator end() { return block_.end(); }
-  const_iterator begin() const { return block_.begin(); }
-  const_iterator end() const { return block_.end(); }
+  iterator begin() { return iterator(block_.begin()); }
+  iterator end() { return iterator(block_.end()); }
+  const_iterator begin() const { return const_iterator(block_.begin()); }
+  const_iterator end() const { return const_iterator(block_.end()); }
   bool empty() const { return block_.empty(); }
   size_t size() const { return block_.size(); }
-  iterator find(base::StringPiece key) { return block_.find(key); }
-  const_iterator find(base::StringPiece key) const { return block_.find(key); }
-  reverse_iterator rbegin() { return block_.rbegin(); }
+  iterator find(base::StringPiece key) { return iterator(block_.find(key)); }
+  const_iterator find(base::StringPiece key) const {
+    return const_iterator(block_.find(key));
+  }
   void erase(base::StringPiece key) { block_.erase(key); }
 
   // Clears both our MapType member and the memory used to hold headers.
@@ -91,7 +170,7 @@ class NET_EXPORT SpdyHeaderBlock {
 
   // If key already exists in the block, replaces the value of that key. Else
   // adds a new header to the end of the block.
-  void insert(const MapType::value_type& value);
+  void insert(const value_type& value);
 
   // If a header with the key is already present, then append the value to the
   // existing header value, NUL ("\0") separated unless the key is cookie, in
@@ -140,8 +219,11 @@ class NET_EXPORT SpdyHeaderBlock {
   };
 
  private:
+  friend class test::SpdyHeaderBlockPeer;
+
   void AppendHeader(const base::StringPiece key, const base::StringPiece value);
   Storage* GetStorage();
+  size_t bytes_allocated() const;
 
   // StringPieces held by |block_| point to memory owned by |*storage_|.
   // |storage_| might be nullptr as long as |block_| is empty.
@@ -149,6 +231,12 @@ class NET_EXPORT SpdyHeaderBlock {
   std::unique_ptr<Storage> storage_;
 };
 
+// Writes |fragments| to |dst|, joined by |separator|. |dst| must be large
+// enough to hold the result. Returns the number of bytes written.
+NET_EXPORT size_t Join(char* dst,
+                       const std::vector<base::StringPiece>& fragments,
+                       base::StringPiece separator);
+
 // Converts a SpdyHeaderBlock into NetLog event parameters.
 NET_EXPORT std::unique_ptr<base::Value> SpdyHeaderBlockNetLogCallback(
     const SpdyHeaderBlock* headers,

net/spdy/spdy_header_block_test.cc

@@ -13,8 +13,10 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using base::StringPiece;
 using std::make_pair;
 using std::string;
+using std::vector;
 using ::testing::ElementsAre;
 
 namespace net {
@@ -22,13 +24,10 @@ namespace test {
 
 class ValueProxyPeer {
  public:
-  static base::StringPiece key(SpdyHeaderBlock::ValueProxy* p) {
-    return p->key_;
-  }
+  static StringPiece key(SpdyHeaderBlock::ValueProxy* p) { return p->key_; }
 };
 
-std::pair<base::StringPiece, base::StringPiece> Pair(base::StringPiece k,
-                                                     base::StringPiece v) {
+std::pair<StringPiece, StringPiece> Pair(StringPiece k, StringPiece v) {
   return make_pair(k, v);
 }
 
@@ -46,12 +45,12 @@ TEST(SpdyHeaderBlockTest, EmptyBlock) {
 
 TEST(SpdyHeaderBlockTest, KeyMemoryReclaimedOnLookup) {
   SpdyHeaderBlock block;
-  base::StringPiece copied_key1;
+  StringPiece copied_key1;
   {
     auto proxy1 = block["some key name"];
     copied_key1 = ValueProxyPeer::key(&proxy1);
   }
-  base::StringPiece copied_key2;
+  StringPiece copied_key2;
   {
     auto proxy2 = block["some other key name"];
     copied_key2 = ValueProxyPeer::key(&proxy2);
@@ -187,12 +186,40 @@ TEST(SpdyHeaderBlockTest, AppendHeaders) {
   block.AppendValueOrAddHeader("h1", "h1v3");
   block.AppendValueOrAddHeader("h2", "h2v3");
   block.AppendValueOrAddHeader("h3", "h3v3");
+  block.AppendValueOrAddHeader("h4", "singleton");
 
   EXPECT_EQ("key1=value1; key2=value2; key3=value3", block["cookie"]);
   EXPECT_EQ("baz", block["foo"]);
   EXPECT_EQ(string("h1v1\0h1v2\0h1v3", 14), block["h1"]);
   EXPECT_EQ(string("h2v1\0h2v2\0h2v3", 14), block["h2"]);
   EXPECT_EQ(string("h3v2\0h3v3", 9), block["h3"]);
+  EXPECT_EQ("singleton", block["h4"]);
+}
+
+TEST(JoinTest, JoinEmpty) {
+  vector<StringPiece> empty;
+  StringPiece separator = ", ";
+  char buf[10] = "";
+  size_t written = Join(buf, empty, separator);
+  EXPECT_EQ(0u, written);
+}
+
+TEST(JoinTest, JoinOne) {
+  vector<StringPiece> v = {"one"};
+  StringPiece separator = ", ";
+  char buf[15];
+  size_t written = Join(buf, v, separator);
+  EXPECT_EQ(3u, written);
+  EXPECT_EQ("one", StringPiece(buf, written));
+}
+
+TEST(JoinTest, JoinMultiple) {
+  vector<StringPiece> v = {"one", "two", "three"};
+  StringPiece separator = ", ";
+  char buf[15];
+  size_t written = Join(buf, v, separator);
+  EXPECT_EQ(15u, written);
+  EXPECT_EQ("one, two, three", StringPiece(buf, written));
 }
 
 }  // namespace test