Use vector for dynamic data in MapBufferBuilder

Summary:
Replaces dynamic manually managed array with a vector of bytes, removing the need for managing memory manually.

This approach is a little bit slower than before, as vector is allocating memory more conservatively. This can be improved in the future by providing an estimate for the data size.

Changelog: [Internal]

Reviewed By: javache

Differential Revision: D33611759

fbshipit-source-id: a0e5e57c4e413206a9f891cd5630ecc9088a9bf7

Author: Andrei Shikov

ReactCommon/react/renderer/mapbuffer/MapBuffer.cpp

@@ -15,15 +15,14 @@ namespace react {
 // TODO T83483191: Extend MapBuffer C++ implementation to support basic random
 // access
 MapBuffer::MapBuffer(std::vector<uint8_t> data) : bytes_(std::move(data)) {
-  count_ = 0;
   memcpy(
       reinterpret_cast<uint8_t *>(&count_),
       bytes_.data() + HEADER_COUNT_OFFSET,
       UINT16_SIZE);
 
   // TODO T83483191: extract memcpy calls into an inline function to simplify
   // the code
-  int32_t dataSize;
+  uint32_t dataSize;
   memcpy(
       reinterpret_cast<uint8_t *>(&dataSize),
       bytes_.data() + HEADER_BUFFER_SIZE_OFFSET,
@@ -113,23 +112,16 @@ bool MapBuffer::isNull(Key key) const {
   return getInt(key) == NULL_VALUE;
 }
 
-int32_t MapBuffer::getBufferSize() const {
+uint32_t MapBuffer::size() const {
   return bytes_.size();
 }
 
-void MapBuffer::copy(uint8_t *output) const {
-  memcpy(output, bytes_.data(), bytes_.size());
+uint8_t const *MapBuffer::data() const {
+  return bytes_.data();
 }
 
-uint16_t MapBuffer::getCount() const {
-  uint16_t size = 0;
-
-  memcpy(
-      reinterpret_cast<uint8_t *>(&size),
-      bytes_.data() + HEADER_COUNT_OFFSET,
-      UINT16_SIZE);
-
-  return size;
+uint16_t MapBuffer::count() const {
+  return count_;
 }
 
 } // namespace react

ReactCommon/react/renderer/mapbuffer/MapBuffer.h

@@ -65,14 +65,13 @@ class MapBuffer {
   // TODO T83483191: review this declaration
   MapBuffer getMapBuffer(Key key) const;
 
-  int32_t getBufferSize() const;
+  bool isNull(Key key) const;
 
-  // TODO T83483191: review parameters of copy method
-  void copy(uint8_t *output) const;
+  uint32_t size() const;
 
-  bool isNull(Key key) const;
+  uint8_t const *data() const;
 
-  uint16_t getCount() const;
+  uint16_t count() const;
 };
 
 } // namespace react

ReactCommon/react/renderer/mapbuffer/MapBufferBuilder.cpp

@@ -13,21 +13,17 @@ namespace facebook {
 namespace react {
 
 MapBuffer MapBufferBuilder::EMPTY() {
-  return MapBufferBuilder().build();
+  return MapBufferBuilder(0).build();
 }
 
 MapBufferBuilder::MapBufferBuilder(uint32_t initialSize) {
   buckets_.reserve(initialSize);
-
-  dynamicDataSize_ = 0;
-  dynamicDataValues_ = nullptr;
-  dynamicDataOffset_ = 0;
 }
 
 void MapBufferBuilder::storeKeyValue(
     Key key,
-    uint8_t *value,
-    int32_t valueSize) {
+    uint8_t const *value,
+    uint32_t valueSize) {
   if (key < minKeyToStore_) {
     LOG(ERROR) << "Error: key out of order - key: " << key;
     abort();
@@ -54,7 +50,7 @@ void MapBufferBuilder::putBool(Key key, bool value) {
 }
 
 void MapBufferBuilder::putDouble(Key key, double value) {
-  uint8_t *bytePointer = reinterpret_cast<uint8_t *>(&value);
+  auto const *bytePointer = reinterpret_cast<uint8_t *>(&value);
   storeKeyValue(key, bytePointer, DOUBLE_SIZE);
 }
 
@@ -63,107 +59,58 @@ void MapBufferBuilder::putNull(Key key) {
 }
 
 void MapBufferBuilder::putInt(Key key, int32_t value) {
-  uint8_t *bytePointer = reinterpret_cast<uint8_t *>(&(value));
+  auto const *bytePointer = reinterpret_cast<uint8_t *>(&(value));
   storeKeyValue(key, bytePointer, INT_SIZE);
 }
 
-void MapBufferBuilder::ensureDynamicDataSpace(int32_t size) {
-  if (dynamicDataValues_ == nullptr) {
-    dynamicDataSize_ = size;
-    dynamicDataValues_ = new Byte[dynamicDataSize_];
-    dynamicDataOffset_ = 0;
-    return;
-  }
-
-  if (dynamicDataOffset_ + size >= dynamicDataSize_) {
-    int32_t oldDynamicDataSize = dynamicDataSize_;
-    react_native_assert(
-        (dynamicDataSize_ < std::numeric_limits<int32_t>::max() / 2) &&
-        "Error: trying to assign a value beyond the capacity of int");
-    dynamicDataSize_ *= dynamicDataSize_;
-
-    react_native_assert(
-        (dynamicDataSize_ < std::numeric_limits<int32_t>::max() - size) &&
-        "Error: trying to assign a value beyond the capacity of int");
-
-    // sum size to ensure that the size always fit into newDynamicDataValues
-    dynamicDataSize_ += size;
-    uint8_t *newDynamicDataValues = new Byte[dynamicDataSize_];
-    uint8_t *oldDynamicDataValues = dynamicDataValues_;
-    memcpy(newDynamicDataValues, dynamicDataValues_, oldDynamicDataSize);
-    dynamicDataValues_ = newDynamicDataValues;
-    delete[] oldDynamicDataValues;
-  }
-}
-
 void MapBufferBuilder::putString(Key key, std::string const &value) {
-  int32_t strLength = static_cast<int32_t>(value.length());
-  const char *cstring = getCstring(&value);
+  int32_t strSize = value.size();
+  const char *strData = value.data();
 
-  // format [lenght of string (int)] + [Array of Characters in the string]
-  int32_t sizeOfLength = INT_SIZE;
-  // TODO T83483191: review if map.getBufferSize() should be an int32_t or long
+  auto offset = dynamicData_.size();
+  // format [length of string (int)] + [Array of Characters in the string]
+  // TODO T83483191: review if map.size() should be an int32_t or long
   // instead of an int16 (because strings can be longer than int16);
-
-  int32_t sizeOfDynamicData = sizeOfLength + strLength;
-  ensureDynamicDataSpace(sizeOfDynamicData);
-  memcpy(dynamicDataValues_ + dynamicDataOffset_, &strLength, sizeOfLength);
-  memcpy(
-      dynamicDataValues_ + dynamicDataOffset_ + sizeOfLength,
-      cstring,
-      strLength);
+  dynamicData_.resize(offset + INT_SIZE + strSize, 0);
+  memcpy(dynamicData_.data() + offset, &strSize, INT_SIZE);
+  memcpy(dynamicData_.data() + offset + INT_SIZE, strData, strSize);
 
   // Store Key and pointer to the string
-  putInt(key, dynamicDataOffset_);
-
-  dynamicDataOffset_ += sizeOfDynamicData;
+  putInt(key, offset);
 }
 
 void MapBufferBuilder::putMapBuffer(Key key, MapBuffer const &map) {
-  int32_t mapBufferSize = map.getBufferSize();
+  int32_t mapBufferSize = map.size();
 
-  // format [lenght of buffer (int)] + [bytes of MapBuffer]
-  int32_t sizeOfDynamicData = mapBufferSize + INT_SIZE;
+  auto offset = dynamicData_.size();
 
-  // format [Array of bytes of the mapBuffer]
-  ensureDynamicDataSpace(sizeOfDynamicData);
-
-  memcpy(dynamicDataValues_ + dynamicDataOffset_, &mapBufferSize, INT_SIZE);
-  // Copy the content of the map into dynamicDataValues_
-  map.copy(dynamicDataValues_ + dynamicDataOffset_ + INT_SIZE);
+  // format [length of buffer (int)] + [bytes of MapBuffer]
+  dynamicData_.resize(offset + INT_SIZE + mapBufferSize, 0);
+  memcpy(dynamicData_.data() + offset, &mapBufferSize, INT_SIZE);
+  // Copy the content of the map into dynamicData_
+  memcpy(dynamicData_.data() + offset + INT_SIZE, map.data(), mapBufferSize);
 
   // Store Key and pointer to the string
-  putInt(key, dynamicDataOffset_);
-
-  dynamicDataOffset_ += sizeOfDynamicData;
+  putInt(key, offset);
 }
 
 MapBuffer MapBufferBuilder::build() {
   // Create buffer: [header] + [key, values] + [dynamic data]
   auto bucketSize = buckets_.size() * BUCKET_SIZE;
-  int32_t bufferSize = HEADER_SIZE + bucketSize + dynamicDataOffset_;
+  uint32_t bufferSize = HEADER_SIZE + bucketSize + dynamicData_.size();
 
   _header.bufferSize = bufferSize;
 
   std::vector<uint8_t> buffer(bufferSize);
   memcpy(buffer.data(), &_header, HEADER_SIZE);
   memcpy(buffer.data() + HEADER_SIZE, buckets_.data(), bucketSize);
-
-  if (dynamicDataValues_ != nullptr) {
-    memcpy(
-        buffer.data() + HEADER_SIZE + bucketSize,
-        dynamicDataValues_,
-        dynamicDataOffset_);
-  }
+  memcpy(
+      buffer.data() + HEADER_SIZE + bucketSize,
+      dynamicData_.data(),
+      dynamicData_.size());
 
   return MapBuffer(std::move(buffer));
 }
 
-MapBufferBuilder::~MapBufferBuilder() {
-  if (dynamicDataValues_ != nullptr) {
-    delete[] dynamicDataValues_;
-  }
-}
-
 } // namespace react
 } // namespace facebook

ReactCommon/react/renderer/mapbuffer/MapBufferBuilder.h

@@ -25,23 +25,11 @@ class MapBufferBuilder {
  private:
   Header _header = {ALIGNMENT, 0, 0};
 
-  void ensureDynamicDataSpace(int32_t size);
-
-  void storeKeyValue(Key key, uint8_t *value, int32_t valueSize);
+  void storeKeyValue(Key key, uint8_t const *value, uint32_t valueSize);
 
   std::vector<Bucket> buckets_{};
 
-  // This array contains data for dynamic values in the MapBuffer.
-  // A dynamic value is a String or another MapBuffer.
-  uint8_t *dynamicDataValues_ = nullptr;
-
-  // Amount of bytes allocated on _dynamicDataValues
-  int32_t dynamicDataSize_ = 0;
-
-  // Relative offset on the _dynamicDataValues array.
-  // This represents the first byte that can be written in _dynamicDataValues
-  // array
-  int32_t dynamicDataOffset_ = 0;
+  std::vector<Byte> dynamicData_{};
 
   // Minimmum key to store in the MapBuffer (this is used to guarantee
   // consistency)
@@ -50,8 +38,6 @@ class MapBufferBuilder {
  public:
   MapBufferBuilder(uint32_t initialSize = INITIAL_BUCKETS_SIZE);
 
-  ~MapBufferBuilder();
-
   static MapBuffer EMPTY();
 
   void putInt(Key key, int32_t value);

ReactCommon/react/renderer/mapbuffer/primitives.h

@@ -27,7 +27,7 @@ namespace react {
 struct Header {
   uint16_t alignment; // alignment of serialization
   uint16_t count; // amount of items in the map
-  int32_t bufferSize; // Amount of bytes used to store the map in memory
+  uint32_t bufferSize; // Amount of bytes used to store the map in memory
 };
 
 static_assert(sizeof(Header) == 8, "MapBuffer header size is incorrect.");
@@ -48,9 +48,10 @@ constexpr static int32_t DOUBLE_SIZE = sizeof(double);
 constexpr static int32_t UINT8_SIZE = sizeof(uint8_t);
 constexpr static int32_t UINT16_SIZE = sizeof(uint16_t);
 constexpr static int32_t UINT64_SIZE = sizeof(uint64_t);
-constexpr static int32_t HEADER_ALIGNMENT_OFFSET = 0;
-constexpr static int32_t HEADER_COUNT_OFFSET = UINT16_SIZE;
-constexpr static int32_t HEADER_BUFFER_SIZE_OFFSET = UINT16_SIZE * 2;
+constexpr static int32_t HEADER_ALIGNMENT_OFFSET = offsetof(Header, alignment);
+constexpr static int32_t HEADER_COUNT_OFFSET = offsetof(Header, count);
+constexpr static int32_t HEADER_BUFFER_SIZE_OFFSET =
+    offsetof(Header, bufferSize);
 
 constexpr static int32_t MAX_VALUE_SIZE = UINT64_SIZE;
 
@@ -71,10 +72,6 @@ inline int32_t getValueOffset(Key key) {
   return getKeyOffset(key) + KEY_SIZE;
 }
 
-static inline const char *getCstring(const std::string *str) {
-  return str ? str->c_str() : "";
-}
-
 inline void
 checkKeyConsistency(const Header &header, const uint8_t *data, Key key) {
 #ifdef CHECK_CONSISTENCY

ReactCommon/react/renderer/mapbuffer/tests/MapBufferTest.cpp

@@ -21,7 +21,7 @@ TEST(MapBufferTest, testSimpleIntMap) {
 
   auto map = builder.build();
 
-  EXPECT_EQ(map.getCount(), 2);
+  EXPECT_EQ(map.count(), 2);
   EXPECT_EQ(map.getInt(0), 1234);
   EXPECT_EQ(map.getInt(1), 4321);
 }
@@ -38,7 +38,7 @@ TEST(MapBufferTest, testMapBufferExtension) {
 
   auto map = buffer.build();
 
-  EXPECT_EQ(map.getCount(), 4);
+  EXPECT_EQ(map.count(), 4);
 
   EXPECT_EQ(map.getInt(0), 1234);
   EXPECT_EQ(map.getInt(1), 4321);
@@ -54,7 +54,7 @@ TEST(MapBufferTest, testBoolEntries) {
 
   auto map = buffer.build();
 
-  EXPECT_EQ(map.getCount(), 2);
+  EXPECT_EQ(map.count(), 2);
   EXPECT_EQ(map.getBool(0), true);
   EXPECT_EQ(map.getBool(1), false);
 }
@@ -67,7 +67,7 @@ TEST(MapBufferTest, testNullEntries) {
 
   auto map = buffer.build();
 
-  EXPECT_EQ(map.getCount(), 2);
+  EXPECT_EQ(map.count(), 2);
   EXPECT_EQ(map.isNull(0), true);
   EXPECT_EQ(map.isNull(1), false);
   // TODO T83483191: serialize null values to be distinguishable from '0'
@@ -84,7 +84,7 @@ TEST(MapBufferTest, testDoubleEntries) {
 
   auto map = buffer.build();
 
-  EXPECT_EQ(map.getCount(), 2);
+  EXPECT_EQ(map.count(), 2);
 
   EXPECT_EQ(map.getDouble(0), 123.4);
   EXPECT_EQ(map.getDouble(1), 432.1);
@@ -134,12 +134,12 @@ TEST(MapBufferTest, testUTFStringEntries) {
 TEST(MapBufferTest, testEmptyMap) {
   auto builder = MapBufferBuilder();
   auto map = builder.build();
-  EXPECT_EQ(map.getCount(), 0);
+  EXPECT_EQ(map.count(), 0);
 }
 
 TEST(MapBufferTest, testEmptyMapConstant) {
   auto map = MapBufferBuilder::EMPTY();
-  EXPECT_EQ(map.getCount(), 0);
+  EXPECT_EQ(map.count(), 0);
 }
 
 TEST(MapBufferTest, testMapEntries) {
@@ -148,7 +148,7 @@ TEST(MapBufferTest, testMapEntries) {
   builder.putInt(1, 1234);
   auto map = builder.build();
 
-  EXPECT_EQ(map.getCount(), 2);
+  EXPECT_EQ(map.count(), 2);
   EXPECT_EQ(map.getString(0), "This is a test");
   EXPECT_EQ(map.getInt(1), 1234);
 
@@ -157,12 +157,12 @@ TEST(MapBufferTest, testMapEntries) {
   builder2.putMapBuffer(1, map);
   auto map2 = builder2.build();
 
-  EXPECT_EQ(map2.getCount(), 2);
+  EXPECT_EQ(map2.count(), 2);
   EXPECT_EQ(map2.getInt(0), 4321);
 
   MapBuffer readMap2 = map2.getMapBuffer(1);
 
-  EXPECT_EQ(readMap2.getCount(), 2);
+  EXPECT_EQ(readMap2.count(), 2);
   EXPECT_EQ(readMap2.getString(0), "This is a test");
   EXPECT_EQ(readMap2.getInt(1), 1234);
 }