Refactor Pickle Read methods to use higher performance PickleIterator.

There was a lot of redundant error checking and initialization code in all Pickle Read methods because of the void** iterator type. This change replaces the void* iterator with PickleIterator, which encapsulates the read pointer so that less error checking and initialization code is needed for reading.

PickleIterator has all the necessary data to do the actual reading. The advantage of having it provide Read methods (as opposed to leaving them solely in the Pickle interface) is that the callers do not need to pass around the const Pickle* once they have a PickleIterator.

Followup CLs will refactor the call sites to remove const Pickle* arguments where they are now unnecessary. Then the Pickle::Read* methods can be removed entirely.

The alternative approach would have been to change the Pickle::Read methods to non-const and remove the iterator parameter (making Read methods advance an internal read pointer). Unfortunately, the const Read with iterator design is entrenched throughout the chromium code, making this a much more complex change with the same performance outcome.

BUG=13108

Review URL: https://chromiumcodereview.appspot.com/9447084

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@125447 0039d316-1c4b-4281-b951-d872f2087c98

1 files changed, 118 insertions, 38 deletions

diff --git a/base/pickle.h b/base/pickle.h
index 65353de..47e3bf6 100644
--- a/base/pickle.h
+++ b/base/pickle.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -10,10 +10,80 @@
 
 #include "base/base_export.h"
 #include "base/basictypes.h"
+#include "base/compiler_specific.h"
 #include "base/gtest_prod_util.h"
 #include "base/logging.h"
 #include "base/string16.h"
 
+class Pickle;
+
+// PickleIterator reads data from a Pickle. The Pickle object must remain valid
+// while the PickleIterator object is in use.
+class BASE_EXPORT PickleIterator {
+ public:
+  PickleIterator() : read_ptr_(NULL), read_end_ptr_(NULL) {}
+  explicit PickleIterator(const Pickle& pickle);
+
+  // Methods for reading the payload of the Pickle. To read from the start of
+  // the Pickle, create a PickleIterator from a Pickle. If successful, these
+  // methods return true. Otherwise, false is returned to indicate that the
+  // result could not be extracted.
+  bool ReadBool(bool* result) WARN_UNUSED_RESULT;
+  bool ReadInt(int* result) WARN_UNUSED_RESULT;
+  bool ReadLong(long* result) WARN_UNUSED_RESULT;
+  bool ReadSize(size_t* result) WARN_UNUSED_RESULT;
+  bool ReadUInt16(uint16* result) WARN_UNUSED_RESULT;
+  bool ReadUInt32(uint32* result) WARN_UNUSED_RESULT;
+  bool ReadInt64(int64* result) WARN_UNUSED_RESULT;
+  bool ReadUInt64(uint64* result) WARN_UNUSED_RESULT;
+  bool ReadString(std::string* result) WARN_UNUSED_RESULT;
+  bool ReadWString(std::wstring* result) WARN_UNUSED_RESULT;
+  bool ReadString16(string16* result) WARN_UNUSED_RESULT;
+  bool ReadData(const char** data, int* length) WARN_UNUSED_RESULT;
+  bool ReadBytes(const char** data, int length) WARN_UNUSED_RESULT;
+
+  // Safer version of ReadInt() checks for the result not being negative.
+  // Use it for reading the object sizes.
+  bool ReadLength(int* result) WARN_UNUSED_RESULT {
+    return ReadInt(result) && *result >= 0;
+  }
+
+  // Skips bytes in the read buffer and returns true if there are at least
+  // num_bytes available. Otherwise, does nothing and returns false.
+  bool SkipBytes(int num_bytes) WARN_UNUSED_RESULT {
+    return !!GetReadPointerAndAdvance(num_bytes);
+  }
+
+ private:
+  // Aligns 'i' by rounding it up to the next multiple of 'alignment'
+  static size_t AlignInt(size_t i, int alignment) {
+    return i + (alignment - (i % alignment)) % alignment;
+  }
+
+  // Read Type from Pickle.
+  template <typename Type>
+  inline bool ReadBuiltinType(Type* result);
+
+  // Get read pointer for Type and advance read pointer.
+  template<typename Type>
+  inline const char* GetReadPointerAndAdvance();
+
+  // Get read pointer for |num_bytes| and advance read pointer. This method
+  // checks num_bytes for negativity and wrapping.
+  const char* GetReadPointerAndAdvance(int num_bytes);
+
+  // Get read pointer for (num_elements * size_element) bytes and advance read
+  // pointer. This method checks for int overflow, negativity and wrapping.
+  inline const char* GetReadPointerAndAdvance(int num_elements,
+                                              size_t size_element);
+
+  // Pointers to the Pickle data.
+  const char* read_ptr_;
+  const char* read_end_ptr_;
+
+  FRIEND_TEST_ALL_PREFIXES(PickleTest, GetReadPointerAndAdvance);
+};
+
 // This class provides facilities for basic binary value packing and unpacking.
 //
 // The Pickle class supports appending primitive values (ints, strings, etc.)
@@ -66,27 +136,54 @@ class BASE_EXPORT Pickle {
   // Returns the data for this Pickle.
   const void* data() const { return header_; }
 
-  // Methods for reading the payload of the Pickle.  To read from the start of
-  // the Pickle, initialize *iter to NULL.  If successful, these methods return
-  // true.  Otherwise, false is returned to indicate that the result could not
-  // be extracted.
-  bool ReadBool(void** iter, bool* result) const;
-  bool ReadInt(void** iter, int* result) const;
-  bool ReadLong(void** iter, long* result) const;
-  bool ReadSize(void** iter, size_t* result) const;
-  bool ReadUInt16(void** iter, uint16* result) const;
-  bool ReadUInt32(void** iter, uint32* result) const;
-  bool ReadInt64(void** iter, int64* result) const;
-  bool ReadUInt64(void** iter, uint64* result) const;
-  bool ReadString(void** iter, std::string* result) const;
-  bool ReadWString(void** iter, std::wstring* result) const;
-  bool ReadString16(void** iter, string16* result) const;
-  bool ReadData(void** iter, const char** data, int* length) const;
-  bool ReadBytes(void** iter, const char** data, int length) const;
+  // For compatibility, these older style read methods pass through to the
+  // PickleIterator methods.
+  // TODO(jbates) Remove these methods.
+  bool ReadBool(PickleIterator* iter, bool* result) const {
+    return iter->ReadBool(result);
+  }
+  bool ReadInt(PickleIterator* iter, int* result) const {
+    return iter->ReadInt(result);
+  }
+  bool ReadLong(PickleIterator* iter, long* result) const {
+    return iter->ReadLong(result);
+  }
+  bool ReadSize(PickleIterator* iter, size_t* result) const {
+    return iter->ReadSize(result);
+  }
+  bool ReadUInt16(PickleIterator* iter, uint16* result) const {
+    return iter->ReadUInt16(result);
+  }
+  bool ReadUInt32(PickleIterator* iter, uint32* result) const {
+    return iter->ReadUInt32(result);
+  }
+  bool ReadInt64(PickleIterator* iter, int64* result) const {
+    return iter->ReadInt64(result);
+  }
+  bool ReadUInt64(PickleIterator* iter, uint64* result) const {
+    return iter->ReadUInt64(result);
+  }
+  bool ReadString(PickleIterator* iter, std::string* result) const {
+    return iter->ReadString(result);
+  }
+  bool ReadWString(PickleIterator* iter, std::wstring* result) const {
+    return iter->ReadWString(result);
+  }
+  bool ReadString16(PickleIterator* iter, string16* result) const {
+    return iter->ReadString16(result);
+  }
+  bool ReadData(PickleIterator* iter, const char** data, int* length) const {
+    return iter->ReadData(data, length);
+  }
+  bool ReadBytes(PickleIterator* iter, const char** data, int length) const {
+    return iter->ReadBytes(data, length);
+  }
 
   // Safer version of ReadInt() checks for the result not being negative.
   // Use it for reading the object sizes.
-  bool ReadLength(void** iter, int* result) const;
+  bool ReadLength(PickleIterator* iter, int* result) const {
+    return iter->ReadLength(result);
+  }
 
   // Methods for adding to the payload of the Pickle.  These values are
   // appended to the end of the Pickle's payload.  When reading values from a
@@ -162,17 +259,6 @@ class BASE_EXPORT Pickle {
     return static_cast<const T*>(header_);
   }
 
-  // Returns true if the given iterator could point to data with the given
-  // length. If there is no room for the given data before the end of the
-  // payload, returns false.
-  bool IteratorHasRoomFor(const void* iter, int len) const {
-    if ((len < 0) || (iter < header_) || iter > end_of_payload())
-      return false;
-    const char* end_of_region = reinterpret_cast<const char*>(iter) + len;
-    // Watch out for overflow in pointer calculation, which wraps.
-    return (iter <= end_of_region) && (end_of_region <= end_of_payload());
-  }
-
  protected:
   size_t payload_size() const { return header_->payload_size; }
 
@@ -220,13 +306,6 @@ class BASE_EXPORT Pickle {
     return i + (alignment - (i % alignment)) % alignment;
   }
 
-  // Moves the iterator by the given number of bytes, making sure it is aligned.
-  // Pointer (iterator) is NOT aligned, but the change in the pointer
-  // is guaranteed to be a multiple of sizeof(uint32).
-  static void UpdateIter(void** iter, int bytes) {
-    *iter = static_cast<char*>(*iter) + AlignInt(bytes, sizeof(uint32));
-  }
-
   // Find the end of the pickled data that starts at range_start.  Returns NULL
   // if the entire Pickle is not found in the given data range.
   static const char* FindNext(size_t header_size,
@@ -237,6 +316,8 @@ class BASE_EXPORT Pickle {
   static const int kPayloadUnit;
 
  private:
+  friend class PickleIterator;
+
   Header* header_;
   size_t header_size_;  // Supports extra data between header and payload.
   // Allocation size of payload (or -1 if allocation is const).
@@ -246,7 +327,6 @@ class BASE_EXPORT Pickle {
   FRIEND_TEST_ALL_PREFIXES(PickleTest, Resize);
   FRIEND_TEST_ALL_PREFIXES(PickleTest, FindNext);
   FRIEND_TEST_ALL_PREFIXES(PickleTest, FindNextWithIncompleteHeader);
-  FRIEND_TEST_ALL_PREFIXES(PickleTest, IteratorHasRoom);
 };
 
 #endif  // BASE_PICKLE_H__