From d1b319fc3f77e744fb325a5a28b6ae448072a9ae Mon Sep 17 00:00:00 2001 From: "piman@chromium.org" Date: Thu, 31 Oct 2013 04:03:02 +0000 Subject: [PATCH] Pickle::Write* micro-optimizations Pickle is hot in some benchmarks. This helps by reworking WriteBytes in a few of ways: 1- Fold BeginWrite and EndWrite into WriteBytes since it's the only caller now. 2- keep track of the write offset, always aligned, separately, so that we can do alignment checks on the field sizes rather than the payload size (for next point). 3- provides a template version of WriteBytes with specializations for predefined sizes, that inline/optimize away alignment checks, padding, and memcpy. 4- change the meaning of capacity_ to not take the header size into account. This simplifies some arithmetic. BUG=307480 R=jar@chromium.org Review URL: https://codereview.chromium.org/34413002 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@231987 0039d316-1c4b-4281-b951-d872f2087c98 --- base/pickle.cc | 117 ++++++++++++++++++++++++------------------------ base/pickle.h | 59 ++++++++++++------------ base/pickle_unittest.cc | 6 +-- 3 files changed, 91 insertions(+), 91 deletions(-) diff --git a/base/pickle.cc b/base/pickle.cc index 1d06af350fa4..c88503ced08e 100644 --- a/base/pickle.cc +++ b/base/pickle.cc @@ -153,7 +153,8 @@ bool PickleIterator::ReadBytes(const char** data, int length) { Pickle::Pickle() : header_(NULL), header_size_(sizeof(Header)), - capacity_(0) { + capacity_after_header_(0), + write_offset_(0) { Resize(kPayloadUnit); header_->payload_size = 0; } @@ -161,7 +162,8 @@ Pickle::Pickle() Pickle::Pickle(int header_size) : header_(NULL), header_size_(AlignInt(header_size, sizeof(uint32))), - capacity_(0) { + capacity_after_header_(0), + write_offset_(0) { DCHECK_GE(static_cast(header_size), sizeof(Header)); DCHECK_LE(header_size, kPayloadUnit); Resize(kPayloadUnit); @@ -171,7 +173,8 @@ Pickle::Pickle(int header_size) Pickle::Pickle(const char* data, size_t data_len) : header_(reinterpret_cast(const_cast(data))), header_size_(0), - capacity_(kCapacityReadOnly) { + capacity_after_header_(kCapacityReadOnly), + write_offset_(0) { if (data_len >= sizeof(Header)) header_size_ = data_len - header_->payload_size; @@ -189,15 +192,15 @@ Pickle::Pickle(const char* data, size_t data_len) Pickle::Pickle(const Pickle& other) : header_(NULL), header_size_(other.header_size_), - capacity_(0) { + capacity_after_header_(0), + write_offset_(other.write_offset_) { size_t payload_size = header_size_ + other.header_->payload_size; - bool resized = Resize(payload_size); - CHECK(resized); // Realloc failed. + Resize(payload_size); memcpy(header_, other.header_, payload_size); } Pickle::~Pickle() { - if (capacity_ != kCapacityReadOnly) + if (capacity_after_header_ != kCapacityReadOnly) free(header_); } @@ -206,19 +209,19 @@ Pickle& Pickle::operator=(const Pickle& other) { NOTREACHED(); return *this; } - if (capacity_ == kCapacityReadOnly) { + if (capacity_after_header_ == kCapacityReadOnly) { header_ = NULL; - capacity_ = 0; + capacity_after_header_ = 0; } if (header_size_ != other.header_size_) { free(header_); header_ = NULL; header_size_ = other.header_size_; } - bool resized = Resize(other.header_size_ + other.header_->payload_size); - CHECK(resized); // Realloc failed. + Resize(other.header_->payload_size); memcpy(header_, other.header_, other.header_size_ + other.header_->payload_size); + write_offset_ = other.write_offset_; return *this; } @@ -249,64 +252,31 @@ bool Pickle::WriteData(const char* data, int length) { return length >= 0 && WriteInt(length) && WriteBytes(data, length); } -bool Pickle::WriteBytes(const void* data, int data_len) { - DCHECK_NE(kCapacityReadOnly, capacity_) << "oops: pickle is readonly"; - - char* dest = BeginWrite(data_len); - if (!dest) - return false; - - memcpy(dest, data, data_len); - - EndWrite(dest, data_len); +bool Pickle::WriteBytes(const void* data, int length) { + WriteBytesCommon(data, length); return true; } -void Pickle::Reserve(size_t additional_capacity) { - // Write at a uint32-aligned offset from the beginning of the header. - size_t offset = AlignInt(header_->payload_size, sizeof(uint32)); - - size_t new_size = offset + additional_capacity; - size_t needed_size = header_size_ + new_size; - if (needed_size > capacity_) - Resize(capacity_ * 2 + needed_size); -} - -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)); - - 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; - +void Pickle::Reserve(size_t length) { + size_t data_len = AlignInt(length, sizeof(uint32)); + DCHECK_GE(data_len, length); #ifdef ARCH_CPU_64_BITS - DCHECK_LE(length, kuint32max); + DCHECK_LE(data_len, kuint32max); #endif - - header_->payload_size = static_cast(new_size); - return mutable_payload() + offset; -} - -void Pickle::EndWrite(char* dest, int length) { - // Zero-pad to keep tools like valgrind from complaining about uninitialized - // memory. - if (length % sizeof(uint32)) - memset(dest + length, 0, sizeof(uint32) - (length % sizeof(uint32))); + DCHECK_LE(write_offset_, kuint32max - data_len); + size_t new_size = write_offset_ + data_len; + if (new_size > capacity_after_header_) + Resize(capacity_after_header_ * 2 + new_size); } -bool Pickle::Resize(size_t new_capacity) { +void Pickle::Resize(size_t new_capacity) { new_capacity = AlignInt(new_capacity, kPayloadUnit); - CHECK_NE(capacity_, kCapacityReadOnly); - void* p = realloc(header_, new_capacity); - if (!p) - return false; - + CHECK_NE(capacity_after_header_, kCapacityReadOnly); + void* p = realloc(header_, header_size_ + new_capacity); + CHECK(p); header_ = reinterpret_cast(p); - capacity_ = new_capacity; - return true; + capacity_after_header_ = new_capacity; } // static @@ -327,3 +297,32 @@ const char* Pickle::FindNext(size_t header_size, return (payload_end > end) ? NULL : payload_end; } + +template void Pickle::WriteBytesStatic(const void* data) { + WriteBytesCommon(data, length); +} + +template void Pickle::WriteBytesStatic<2>(const void* data); +template void Pickle::WriteBytesStatic<4>(const void* data); +template void Pickle::WriteBytesStatic<8>(const void* data); + +inline void Pickle::WriteBytesCommon(const void* data, size_t length) { + DCHECK_NE(kCapacityReadOnly, capacity_after_header_) + << "oops: pickle is readonly"; + size_t data_len = AlignInt(length, sizeof(uint32)); + DCHECK_GE(data_len, length); +#ifdef ARCH_CPU_64_BITS + DCHECK_LE(data_len, kuint32max); +#endif + DCHECK_LE(write_offset_, kuint32max - data_len); + size_t new_size = write_offset_ + data_len; + if (new_size > capacity_after_header_) { + Resize(std::max(capacity_after_header_ * 2, new_size)); + } + + char* write = mutable_payload() + write_offset_; + memcpy(write, data, length); + memset(write + length, 0, data_len - length); + header_->payload_size = static_cast(write_offset_ + length); + write_offset_ = new_size; +} diff --git a/base/pickle.h b/base/pickle.h index 41a673380e07..952ef0dae539 100644 --- a/base/pickle.h +++ b/base/pickle.h @@ -216,7 +216,7 @@ class BASE_EXPORT Pickle { return WriteInt(value ? 1 : 0); } bool WriteInt(int value) { - return WriteBytes(&value, sizeof(value)); + return WritePOD(value); } // WARNING: DO NOT USE THIS METHOD IF PICKLES ARE PERSISTED IN ANY WAY. // It will write whatever a "long" is on this architecture. On 32-bit @@ -224,22 +224,22 @@ class BASE_EXPORT Pickle { // pickles are still around after upgrading to 64-bit, or if they are copied // between dissimilar systems, YOUR PICKLES WILL HAVE GONE BAD. bool WriteLongUsingDangerousNonPortableLessPersistableForm(long value) { - return WriteBytes(&value, sizeof(value)); + return WritePOD(value); } bool WriteUInt16(uint16 value) { - return WriteBytes(&value, sizeof(value)); + return WritePOD(value); } bool WriteUInt32(uint32 value) { - return WriteBytes(&value, sizeof(value)); + return WritePOD(value); } bool WriteInt64(int64 value) { - return WriteBytes(&value, sizeof(value)); + return WritePOD(value); } bool WriteUInt64(uint64 value) { - return WriteBytes(&value, sizeof(value)); + return WritePOD(value); } bool WriteFloat(float value) { - return WriteBytes(&value, sizeof(value)); + return WritePOD(value); } bool WriteString(const std::string& value); bool WriteWString(const std::wstring& value); @@ -247,10 +247,10 @@ class BASE_EXPORT Pickle { // "Data" is a blob with a length. When you read it out you will be given the // length. See also WriteBytes. bool WriteData(const char* data, int length); - // "Bytes" is a blob with no length. The caller must specify the lenght both + // "Bytes" is a blob with no length. The caller must specify the length both // when reading and writing. It is normally used to serialize PoD types of a // known size. See also WriteData. - bool WriteBytes(const void* data, int data_len); + bool WriteBytes(const void* data, int length); // Reserves space for upcoming writes when multiple writes will be made and // their sizes are computed in advance. It can be significantly faster to call @@ -295,26 +295,13 @@ class BASE_EXPORT Pickle { return reinterpret_cast(header_) + header_size_; } - size_t capacity() const { - return capacity_; + size_t capacity_after_header() const { + return capacity_after_header_; } - // Resizes the buffer for use when writing the specified amount of data. The - // location that the data should be written at is returned, or NULL if there - // was an error. Call EndWrite with the returned offset and the given length - // to pad out for the next write. - char* BeginWrite(size_t length); - - // Completes the write operation by padding the data with NULL bytes until it - // is padded. Should be paired with BeginWrite, but it does not necessarily - // have to be called after the data is written. - void EndWrite(char* dest, int length); - - // Resize the capacity, note that the input value should include the size of - // the header: new_capacity = sizeof(Header) + desired_payload_capacity. - // A realloc() failure will cause a Resize failure... and caller should check - // the return result for true (i.e., successful resizing). - bool Resize(size_t new_capacity); + // Resize the capacity, note that the input value should not include the size + // of the header. + void Resize(size_t new_capacity); // Aligns 'i' by rounding it up to the next multiple of 'alignment' static size_t AlignInt(size_t i, int alignment) { @@ -335,8 +322,22 @@ class BASE_EXPORT Pickle { Header* header_; size_t header_size_; // Supports extra data between header and payload. - // Allocation size of payload (or -1 if allocation is const). - size_t capacity_; + // Allocation size of payload (or -1 if allocation is const). Note: this + // doesn't count the header. + size_t capacity_after_header_; + // The offset at which we will write the next field. Note: this doesn't count + // the header. + size_t write_offset_; + + // Just like WriteBytes, but with a compile-time size, for performance. + template void WriteBytesStatic(const void* data); + + // Writes a POD by copying its bytes. + template bool WritePOD(const T& data) { + WriteBytesStatic(&data); + return true; + } + inline void WriteBytesCommon(const void* data, size_t length); FRIEND_TEST_ALL_PREFIXES(PickleTest, Resize); FRIEND_TEST_ALL_PREFIXES(PickleTest, FindNext); diff --git a/base/pickle_unittest.cc b/base/pickle_unittest.cc index dd00e6a5d4db..9dd009116ca0 100644 --- a/base/pickle_unittest.cc +++ b/base/pickle_unittest.cc @@ -217,19 +217,19 @@ TEST(PickleTest, Resize) { size_t cur_payload = payload_size_after_header; // note: we assume 'unit' is a power of 2 - EXPECT_EQ(unit, pickle.capacity()); + EXPECT_EQ(unit, pickle.capacity_after_header()); EXPECT_EQ(pickle.payload_size(), payload_size_after_header); // fill out a full page (noting data header) pickle.WriteData(data_ptr, static_cast(unit - sizeof(uint32))); cur_payload += unit; - EXPECT_EQ(unit * 2, pickle.capacity()); + EXPECT_EQ(unit * 2, pickle.capacity_after_header()); EXPECT_EQ(cur_payload, pickle.payload_size()); // one more byte should double the capacity pickle.WriteData(data_ptr, 1); cur_payload += 5; - EXPECT_EQ(unit * 4, pickle.capacity()); + EXPECT_EQ(unit * 4, pickle.capacity_after_header()); EXPECT_EQ(cur_payload, pickle.payload_size()); } -- 2.11.4.GIT