Pickle::Write* micro-optimizations

base/pickle.cc

Index: base/pickle.cc
diff --git a/base/pickle.cc b/base/pickle.cc
index af3191b9d86db2785fe375ecbcb045cf0a420870..a61140d3dd31d37f1b772b14be832016cb3d2437 100644
--- a/base/pickle.cc
+++ b/base/pickle.cc
@@ -154,7 +154,8 @@ Pickle::Pickle()
     : header_(NULL),
       header_size_(sizeof(Header)),
       capacity_(0),
-      variable_buffer_offset_(0) {
+      variable_buffer_offset_(0),
+      offset_(0) {
   Resize(kPayloadUnit);
   header_->payload_size = 0;
 }
@@ -163,7 +164,8 @@ Pickle::Pickle(int header_size)
     : header_(NULL),
       header_size_(AlignInt(header_size, sizeof(uint32))),
       capacity_(0),
-      variable_buffer_offset_(0) {
+      variable_buffer_offset_(0),
+      offset_(0) {
   DCHECK_GE(static_cast<size_t>(header_size), sizeof(Header));
   DCHECK_LE(header_size, kPayloadUnit);
   Resize(kPayloadUnit);
@@ -174,7 +176,8 @@ Pickle::Pickle(const char* data, int data_len)
     : header_(reinterpret_cast<Header*>(const_cast<char*>(data))),
       header_size_(0),
       capacity_(kCapacityReadOnly),
-      variable_buffer_offset_(0) {
+      variable_buffer_offset_(0),
+      offset_(0) {
   if (data_len >= static_cast<int>(sizeof(Header)))
     header_size_ = data_len - header_->payload_size;
 
@@ -193,7 +196,8 @@ Pickle::Pickle(const Pickle& other)
     : header_(NULL),
       header_size_(other.header_size_),
       capacity_(0),
-      variable_buffer_offset_(other.variable_buffer_offset_) {
+      variable_buffer_offset_(other.variable_buffer_offset_),
+      offset_(0) {
   size_t payload_size = header_size_ + other.header_->payload_size;
   bool resized = Resize(payload_size);
   CHECK(resized);  // Realloc failed.
@@ -304,21 +308,26 @@ void Pickle::TrimWriteData(int new_length) {
   *cur_length = new_length;
 }
 
-char* Pickle::BeginWrite(size_t length) {
-  // write at a uint32-aligned offset from the beginning of the header
-  size_t offset = AlignInt(header_->payload_size, sizeof(uint32));
+void Pickle::Reserve(size_t length) {
+  size_t needed_size = header_->payload_size + length;
+  if (needed_size > capacity_)
+    Resize(std::max(capacity_ * 2, needed_size));

[danakj, 2013/10/22 17:22:05]

This will generally cause 2 resizes instead of one, since the needed_size is
going to be filled and there will usually be another field after? What about
resizing larger than needed_size?

[danakj, 2013/10/22 17:23:16]

Maybe add them instead of max?

[danakj, 2013/10/22 18:23:54]

I added another test with 1000 render passes instead of one.
Doing max here: 19195us
Doing + here: 16344

[piman, 2013/10/24 06:17:14]

I rebased on top of your patch (that does this).

+}
 
-  size_t new_size = offset + length;
-  size_t needed_size = header_size_ + new_size;
-  if (needed_size > capacity_ && !Resize(std::max(capacity_ * 2, needed_size)))
-    return NULL;

[piman, 2013/10/22 06:05:58]

behavior change: Resize can only fail when realloc fails, but in this case
tcmalloc should abort anyway. It adds significant savings.

+char* Pickle::BeginWrite(size_t length) {
+  size_t data_len = AlignInt(length, sizeof(uint32));
+  size_t new_size = header_->payload_size + data_len;
+  if (new_size > capacity_)
+    Resize(std::max(capacity_ * 2, new_size));
 
 #ifdef ARCH_CPU_64_BITS
   DCHECK_LE(length, kuint32max);
 #endif
 
-  header_->payload_size = static_cast<uint32>(new_size);
-  return mutable_payload() + offset;
+  char* data = mutable_payload() + header_->payload_size;
+  header_->payload_size = new_size;

[piman, 2013/10/22 06:05:58]

behavior change: header_->payload_size now counts the padding bytes. That may be
iffy, but I think I can fix it.

[piman, 2013/10/24 06:17:14]

I fixed this.

+  offset_ = new_size;
+  return data;
 }
 
 void Pickle::EndWrite(char* dest, int length) {
@@ -332,7 +341,7 @@ bool Pickle::Resize(size_t new_capacity) {
   new_capacity = AlignInt(new_capacity, kPayloadUnit);
 
   CHECK_NE(capacity_, kCapacityReadOnly);
-  void* p = realloc(header_, new_capacity);
+  void* p = realloc(header_, new_capacity + header_size_);
   if (!p)
     return false;
 
@@ -359,3 +368,27 @@ const char* Pickle::FindNext(size_t header_size,
 
   return (payload_end > end) ? NULL : payload_end;
 }
+
+#if 1
+template <size_t length> void Pickle::WriteBytesStatic(const void* data) {
+  DCHECK_NE(kCapacityReadOnly, capacity_) << "oops: pickle is readonly";
+#ifdef ARCH_CPU_64_BITS
+  DCHECK_LE(length, kuint32max);
+#endif
+  size_t data_len = AlignInt(length, sizeof(uint32));

[piman, 2013/10/22 06:05:58]

This gets optimized out for length%4==0

+  size_t new_size = offset_ + data_len;
+  if (new_size > capacity_) {
+    Resize(std::max(capacity_ * 2, new_size));

[piman, 2013/10/22 06:05:58]

I tried to amortize the cost of this with the Reserve(), but just the if test
has a cost.

[danakj, 2013/10/22 17:22:05]

If Reserve gave you a buffer larger than just what you requested, could you use
that instead?

+  }
+
+  char* write = mutable_payload() + offset_;
+  memcpy(write, data, length);
+  memset(write + length, 0, data_len - length);

[piman, 2013/10/22 06:05:58]

This gets optimized out too for length%4==0

+  offset_ = new_size;
+  header_->payload_size = new_size;
+}
+
+template void Pickle::WriteBytesStatic<2>(const void* data);
+template void Pickle::WriteBytesStatic<4>(const void* data);
+template void Pickle::WriteBytesStatic<8>(const void* data);
+#endif