Use struct for building mapbuffer buckets

Summary:
Instead of copying integer values into key/value data array, this implementation keeps a structured vector of `Bucket` structs (which is sized to be serialized later).

This approach provides multiple benefits to modify and access buckets (e.g. sort them based on the key, allowing for out of order inserts (D33611758))

Changelog: [Internal]

Reviewed By: javache

Differential Revision: D33601231

fbshipit-source-id: 62ace1374936cb504836d6eae672e909ea404e3f

Author: Andrei Shikov

ReactCommon/react/renderer/mapbuffer/MapBufferBuilder.cpp

@@ -12,38 +12,18 @@ using namespace facebook::react;
 namespace facebook {
 namespace react {
 
-// TODO T83483191: Add asserts to check overflowing on additions
-MapBufferBuilder::MapBufferBuilder()
-    : MapBufferBuilder::MapBufferBuilder(INITIAL_KEY_VALUE_SIZE) {}
-
 MapBuffer MapBufferBuilder::EMPTY() {
   return MapBufferBuilder().build();
 }
 
-MapBufferBuilder::MapBufferBuilder(uint16_t initialSize) {
-  keyValuesSize_ = initialSize;
-  keyValues_ = new Byte[keyValuesSize_];
-  // First Key should be written right after the header.
-  keyValuesOffset_ = HEADER_SIZE;
+MapBufferBuilder::MapBufferBuilder(uint32_t initialSize) {
+  buckets_.reserve(initialSize);
 
   dynamicDataSize_ = 0;
   dynamicDataValues_ = nullptr;
   dynamicDataOffset_ = 0;
 }
 
-void MapBufferBuilder::ensureKeyValueSpace() {
-  int32_t oldKeyValuesSize = keyValuesSize_;
-  react_native_assert(
-      (keyValuesSize_ < std::numeric_limits<uint16_t>::max() / 2) &&
-      "Error trying to assign a value beyond the capacity of uint16_t: ");
-  keyValuesSize_ *= 2;
-  uint8_t *newKeyValues = new Byte[keyValuesSize_];
-  uint8_t *oldKeyValues = keyValues_;
-  memcpy(newKeyValues, keyValues_, oldKeyValuesSize);
-  keyValues_ = newKeyValues;
-  delete[] oldKeyValues;
-}
-
 void MapBufferBuilder::storeKeyValue(
     Key key,
     uint8_t *value,
@@ -58,24 +38,15 @@ void MapBufferBuilder::storeKeyValue(
     abort();
   }
 
-  // TODO T83483191: header.count points to the next index
-  // TODO T83483191: add test to verify storage of sparse keys
-  int32_t keyOffset = getKeyOffset(_header.count);
-  int32_t valueOffset = keyOffset + KEY_SIZE;
-
-  int32_t nextKeyValueOffset = keyOffset + BUCKET_SIZE;
-  if (nextKeyValueOffset >= keyValuesSize_) {
-    ensureKeyValueSpace();
-  }
+  uint64_t data = 0;
+  auto *dataPtr = reinterpret_cast<uint8_t *>(&data);
+  memcpy(dataPtr, value, valueSize);
 
-  memcpy(keyValues_ + keyOffset, &key, KEY_SIZE);
-  memcpy(keyValues_ + valueOffset, value, valueSize);
+  buckets_.emplace_back(key, data);
 
   _header.count++;
 
   minKeyToStore_ = key + 1;
-  // Move keyValuesOffset_ to the next available [key, value] position
-  keyValuesOffset_ = std::max(nextKeyValueOffset, keyValuesOffset_);
 }
 
 void MapBufferBuilder::putBool(Key key, bool value) {
@@ -168,56 +139,27 @@ void MapBufferBuilder::putMapBuffer(Key key, MapBuffer const &map) {
 }
 
 MapBuffer MapBufferBuilder::build() {
-  react_native_assert(
-      (keyValues_ != nullptr) &&
-      "Error when building mapbuffer with invalid datastructures.");
-
   // Create buffer: [header] + [key, values] + [dynamic data]
-  int32_t bufferSize = keyValuesOffset_ + dynamicDataOffset_;
+  auto bucketSize = buckets_.size() * BUCKET_SIZE;
+  int32_t bufferSize = HEADER_SIZE + bucketSize + dynamicDataOffset_;
 
   _header.bufferSize = bufferSize;
 
-  // Copy header at the beginning of "keyValues_"
-  memcpy(keyValues_, &_header, HEADER_SIZE);
-
   std::vector<uint8_t> buffer(bufferSize);
-
-  memcpy(buffer.data(), keyValues_, keyValuesOffset_);
+  memcpy(buffer.data(), &_header, HEADER_SIZE);
+  memcpy(buffer.data() + HEADER_SIZE, buckets_.data(), bucketSize);
 
   if (dynamicDataValues_ != nullptr) {
     memcpy(
-        buffer.data() + keyValuesOffset_,
+        buffer.data() + HEADER_SIZE + bucketSize,
         dynamicDataValues_,
         dynamicDataOffset_);
   }
 
-  auto map = MapBuffer(std::move(buffer));
-
-  // TODO T83483191: we should invalidate the class once the build() method is
-  // called.
-
-  if (keyValues_ != nullptr) {
-    delete[] keyValues_;
-  }
-  keyValues_ = nullptr;
-  keyValuesSize_ = 0;
-  keyValuesOffset_ = 0;
-
-  if (dynamicDataValues_ != nullptr) {
-    delete[] dynamicDataValues_;
-    dynamicDataValues_ = nullptr;
-  }
-  dynamicDataSize_ = 0;
-  dynamicDataOffset_ = 0;
-  _header = {ALIGNMENT, 0, 0};
-
-  return map;
+  return MapBuffer(std::move(buffer));
 }
 
 MapBufferBuilder::~MapBufferBuilder() {
-  if (keyValues_ != nullptr) {
-    delete[] keyValues_;
-  }
   if (dynamicDataValues_ != nullptr) {
     delete[] dynamicDataValues_;
   }

ReactCommon/react/renderer/mapbuffer/MapBufferBuilder.h

@@ -10,17 +10,13 @@
 #include <react/debug/react_native_assert.h>
 #include <react/renderer/mapbuffer/MapBuffer.h>
 #include <react/renderer/mapbuffer/primitives.h>
-#include <stdlib.h>
+#include <cstdlib>
 
 namespace facebook {
 namespace react {
 
-// Default initial size for _keyValues array
-// 108 = 10 entries = 10*10 + 8 sizeof(header)
-constexpr uint16_t INITIAL_KEY_VALUE_SIZE = 108;
-
-// Default initial size for _dynamicDataValues array
-constexpr int32_t INITIAL_DYNAMIC_DATA_SIZE = 200;
+// Default reserved size for buckets_ vector
+constexpr uint16_t INITIAL_BUCKETS_SIZE = 10;
 
 /**
  * MapBufferBuilder is a builder class for MapBuffer
@@ -29,25 +25,11 @@ class MapBufferBuilder {
  private:
   Header _header = {ALIGNMENT, 0, 0};
 
-  void ensureKeyValueSpace();
-
   void ensureDynamicDataSpace(int32_t size);
 
   void storeKeyValue(Key key, uint8_t *value, int32_t valueSize);
 
-  // Array of [key,value] map entries:
-  // - Key is represented in 2 bytes
-  // - Value is stored into 8 bytes. The 8 bytes of the value will contain the
-  // actual value for the key or a pointer to the actual value (based on the
-  // type)
-  uint8_t *keyValues_ = nullptr;
-
-  // Amount of bytes allocated on _keyValues
-  uint16_t keyValuesSize_ = 0;
-
-  // Relative offset on the _keyValues array.
-  // This represents the first byte that can be written in _keyValues array
-  int32_t keyValuesOffset_ = 0;
+  std::vector<Bucket> buckets_{};
 
   // This array contains data for dynamic values in the MapBuffer.
   // A dynamic value is a String or another MapBuffer.
@@ -66,9 +48,7 @@ class MapBufferBuilder {
   uint16_t minKeyToStore_ = 0;
 
  public:
-  MapBufferBuilder();
-
-  MapBufferBuilder(uint16_t initialSize);
+  MapBufferBuilder(uint32_t initialSize = INITIAL_BUCKETS_SIZE);
 
   ~MapBufferBuilder();
 

ReactCommon/react/renderer/mapbuffer/primitives.h

@@ -30,6 +30,17 @@ struct Header {
   int32_t bufferSize; // Amount of bytes used to store the map in memory
 };
 
+static_assert(sizeof(Header) == 8, "MapBuffer header size is incorrect.");
+
+struct __attribute__((__packed__)) Bucket {
+  Key key;
+  uint64_t data;
+
+  Bucket(Key key, uint64_t data) : key(key), data(data){};
+};
+
+static_assert(sizeof(Bucket) == 10, "MapBuffer bucket size is incorrect.");
+
 constexpr static int32_t KEY_SIZE = sizeof(Key);
 constexpr static int32_t HEADER_SIZE = sizeof(Header);
 constexpr static int32_t INT_SIZE = sizeof(int32_t);