Allow random stores and accesses to MapBuffer values

Summary:
Removes the need to store keys in incremental order by sorting them after before inserting into MapBuffer.
Updates MapBuffer to support random access on C++ side, actually retrieving values by keys and not bucket index.

Changelog: [Internal]

Reviewed By: mdvacca

Differential Revision: D33611758

fbshipit-source-id: c81e970613c8ecc688bfacb29ba038bf081a0c0f

ReactCommon/react/renderer/mapbuffer/MapBuffer.cpp

@@ -12,6 +12,14 @@ using namespace facebook::react;
 namespace facebook {
 namespace react {
 
+static inline int32_t bucketOffset(int32_t index) {
+  return HEADER_SIZE + BUCKET_SIZE * index;
+}
+
+static inline int32_t valueOffset(int32_t bucketIndex) {
+  return bucketOffset(bucketIndex) + offsetof(Bucket, data);
+}
+
 // TODO T83483191: Extend MapBuffer C++ implementation to support basic random
 // access
 MapBuffer::MapBuffer(std::vector<uint8_t> data) : bytes_(std::move(data)) {
@@ -25,13 +33,33 @@ MapBuffer::MapBuffer(std::vector<uint8_t> data) : bytes_(std::move(data)) {
   }
 }
 
+uint32_t MapBuffer::getKeyBucket(Key key) const {
+  uint32_t lo = 0;
+  uint32_t hi = count_ - 1;
+  while (lo <= hi) {
+    uint32_t mid = (lo + hi) >> 1;
+
+    Key midVal =
+        *reinterpret_cast<Key const *>(bytes_.data() + bucketOffset(mid));
+
+    if (midVal < key) {
+      lo = mid + 1;
+    } else if (midVal > key) {
+      hi = mid - 1;
+    } else {
+      return mid;
+    }
+  }
+
+  return -1;
+}
+
 int32_t MapBuffer::getInt(Key key) const {
-  int32_t value = 0;
-  memcpy(
-      reinterpret_cast<uint8_t *>(&value),
-      bytes_.data() + getValueOffset(key),
-      INT_SIZE);
-  return value;
+  auto bucketIndex = getKeyBucket(key);
+  react_native_assert(bucketIndex != -1 && "Key not found in MapBuffer");
+
+  return *reinterpret_cast<int32_t const *>(
+      bytes_.data() + valueOffset(bucketIndex));
 }
 
 bool MapBuffer::getBool(Key key) const {
@@ -39,41 +67,31 @@ bool MapBuffer::getBool(Key key) const {
 }
 
 double MapBuffer::getDouble(Key key) const {
-  // TODO T83483191: extract this code into a "template method" and reuse it for
-  // other types
-  double value = 0;
-  memcpy(
-      reinterpret_cast<uint8_t *>(&value),
-      bytes_.data() + getValueOffset(key),
-      DOUBLE_SIZE);
-  return value;
+  auto bucketIndex = getKeyBucket(key);
+  react_native_assert(bucketIndex != -1 && "Key not found in MapBuffer");
+
+  return *reinterpret_cast<double const *>(
+      bytes_.data() + valueOffset(bucketIndex));
+  ;
 }
 
 int32_t MapBuffer::getDynamicDataOffset() const {
-  // The begininig of dynamic data can be calculated as the offset of the next
+  // The start of dynamic data can be calculated as the offset of the next
   // key in the map
-  return getKeyOffset(count_);
+  return bucketOffset(count_);
 }
 
 std::string MapBuffer::getString(Key key) const {
   // TODO T83483191:Add checks to verify that offsets are under the boundaries
   // of the map buffer
   int32_t dynamicDataOffset = getDynamicDataOffset();
-  int32_t stringLength = 0;
   int32_t offset = getInt(key);
-  memcpy(
-      reinterpret_cast<uint8_t *>(&stringLength),
-      bytes_.data() + dynamicDataOffset + offset,
-      INT_SIZE);
+  int32_t stringLength = *reinterpret_cast<int32_t const *>(
+      bytes_.data() + dynamicDataOffset + offset);
+  uint8_t const *stringPtr =
+      bytes_.data() + dynamicDataOffset + offset + INT_SIZE;
 
-  char *value = new char[stringLength];
-
-  memcpy(
-      reinterpret_cast<char *>(value),
-      bytes_.data() + dynamicDataOffset + offset + INT_SIZE,
-      stringLength);
-
-  return std::string(value, 0, stringLength);
+  return std::string(stringPtr, stringPtr + stringLength);
 }
 
 MapBuffer MapBuffer::getMapBuffer(Key key) const {
@@ -81,12 +99,9 @@ MapBuffer MapBuffer::getMapBuffer(Key key) const {
   // of the map buffer
   int32_t dynamicDataOffset = getDynamicDataOffset();
 
-  int32_t mapBufferLength = 0;
   int32_t offset = getInt(key);
-  memcpy(
-      reinterpret_cast<uint8_t *>(&mapBufferLength),
-      bytes_.data() + dynamicDataOffset + offset,
-      INT_SIZE);
+  int32_t mapBufferLength = *reinterpret_cast<int32_t const *>(
+      bytes_.data() + dynamicDataOffset + offset);
 
   std::vector<uint8_t> value(mapBufferLength);
 

ReactCommon/react/renderer/mapbuffer/MapBuffer.h

@@ -45,6 +45,8 @@ class MapBuffer {
   // returns the relative offset of the first byte of dynamic data
   int32_t getDynamicDataOffset() const;
 
+  uint32_t getKeyBucket(Key key) const;
+
  public:
   explicit MapBuffer(std::vector<uint8_t> data);
 

ReactCommon/react/renderer/mapbuffer/MapBufferBuilder.cpp

@@ -6,6 +6,7 @@
  */
 
 #include "MapBufferBuilder.h"
+#include <algorithm>
 
 using namespace facebook::react;
 
@@ -24,10 +25,6 @@ void MapBufferBuilder::storeKeyValue(
     Key key,
     uint8_t const *value,
     uint32_t valueSize) {
-  if (key < minKeyToStore_) {
-    LOG(ERROR) << "Error: key out of order - key: " << key;
-    abort();
-  }
   if (valueSize > MAX_VALUE_SIZE) {
     LOG(ERROR) << "Error: size of value must be <= MAX_VALUE_SIZE. ValueSize: "
                << valueSize;
@@ -42,7 +39,10 @@ void MapBufferBuilder::storeKeyValue(
 
   header_.count++;
 
-  minKeyToStore_ = key + 1;
+  if (lastKey_ > key) {
+    needsSort_ = true;
+  }
+  lastKey_ = key;
 }
 
 void MapBufferBuilder::putBool(Key key, bool value) {
@@ -67,10 +67,8 @@ void MapBufferBuilder::putString(Key key, std::string const &value) {
   int32_t strSize = value.size();
   const char *strData = value.data();
 
-  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);
+  auto offset = dynamicData_.size();
   dynamicData_.resize(offset + INT_SIZE + strSize, 0);
   memcpy(dynamicData_.data() + offset, &strSize, INT_SIZE);
   memcpy(dynamicData_.data() + offset + INT_SIZE, strData, strSize);
@@ -94,6 +92,10 @@ void MapBufferBuilder::putMapBuffer(Key key, MapBuffer const &map) {
   putInt(key, offset);
 }
 
+static inline bool compareBuckets(Bucket const &a, Bucket const &b) {
+  return a.key < b.key;
+}
+
 MapBuffer MapBufferBuilder::build() {
   // Create buffer: [header] + [key, values] + [dynamic data]
   auto bucketSize = buckets_.size() * BUCKET_SIZE;
@@ -101,6 +103,12 @@ MapBuffer MapBufferBuilder::build() {
 
   header_.bufferSize = bufferSize;
 
+  if (needsSort_) {
+    std::sort(buckets_.begin(), buckets_.end(), compareBuckets);
+  }
+
+  // TODO(T83483191): add pass to check for duplicates
+
   std::vector<uint8_t> buffer(bufferSize);
   memcpy(buffer.data(), &header_, HEADER_SIZE);
   memcpy(buffer.data() + HEADER_SIZE, buckets_.data(), bucketSize);

ReactCommon/react/renderer/mapbuffer/MapBufferBuilder.h

@@ -22,19 +22,6 @@ constexpr uint16_t INITIAL_BUCKETS_SIZE = 10;
  * MapBufferBuilder is a builder class for MapBuffer
  */
 class MapBufferBuilder {
- private:
-  Header header_ = {ALIGNMENT, 0, 0};
-
-  void storeKeyValue(Key key, uint8_t const *value, uint32_t valueSize);
-
-  std::vector<Bucket> buckets_{};
-
-  std::vector<Byte> dynamicData_{};
-
-  // Minimmum key to store in the MapBuffer (this is used to guarantee
-  // consistency)
-  uint16_t minKeyToStore_ = 0;
-
  public:
   MapBufferBuilder(uint32_t initialSize = INITIAL_BUCKETS_SIZE);
 
@@ -52,8 +39,20 @@ class MapBufferBuilder {
 
   void putMapBuffer(Key key, MapBuffer const &map);
 
-  // TODO T83483191: This should return MapBuffer!
   MapBuffer build();
+
+ private:
+  Header header_ = {ALIGNMENT, 0, 0};
+
+  std::vector<Bucket> buckets_{};
+
+  std::vector<Byte> dynamicData_{};
+
+  uint16_t lastKey_{0};
+
+  bool needsSort_{false};
+
+  void storeKeyValue(Key key, uint8_t const *value, uint32_t valueSize);
 };
 
 } // namespace react

ReactCommon/react/renderer/mapbuffer/primitives.h

@@ -56,21 +56,7 @@ constexpr static int32_t HEADER_BUFFER_SIZE_OFFSET =
 constexpr static int32_t MAX_VALUE_SIZE = UINT64_SIZE;
 
 // 10 bytes : 2 key + 8 value
-constexpr static int32_t BUCKET_SIZE = KEY_SIZE + UINT64_SIZE;
-
-/**
- * Returns the offset of the key received by parameter.
- */
-inline int32_t getKeyOffset(Key key) {
-  return HEADER_SIZE + BUCKET_SIZE * key;
-}
-
-/**
- * Returns the offset of the value associated to the key received by parameter.
- */
-inline int32_t getValueOffset(Key key) {
-  return getKeyOffset(key) + KEY_SIZE;
-}
+constexpr static int32_t BUCKET_SIZE = sizeof(Bucket);
 
 inline void
 checkKeyConsistency(const Header &header, const uint8_t *data, Key key) {

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

@@ -166,3 +166,20 @@ TEST(MapBufferTest, testMapEntries) {
   EXPECT_EQ(readMap2.getString(0), "This is a test");
   EXPECT_EQ(readMap2.getInt(1), 1234);
 }
+
+TEST(MapBufferTest, testMapRandomAccess) {
+  auto builder = MapBufferBuilder();
+  builder.putInt(1234, 4321);
+  builder.putString(0, "This is a test");
+  builder.putDouble(8, 908.1);
+  builder.putNull(2);
+  builder.putString(65535, "Let's count: 的, 一, 是");
+  auto map = builder.build();
+
+  EXPECT_EQ(map.count(), 5);
+  EXPECT_EQ(map.getString(0), "This is a test");
+  EXPECT_EQ(map.getDouble(8), 908.1);
+  EXPECT_EQ(map.isNull(2), true);
+  EXPECT_EQ(map.getInt(1234), 4321);
+  EXPECT_EQ(map.getString(65535), "Let's count: 的, 一, 是");
+}