Cleanup ReadableMapBuffer

Summary:
Cleans up `ReadableMapBuffer` APIs and migrates to use `std::vector` instead of raw pointer array.

Also uses `fbjni` utility to allocate the bytes instead of making manual JNI calls.

Changelog: [Internal]

Reviewed By: sammy-SC

Differential Revision: D33513027

fbshipit-source-id: afe7d320d12830503de4c9994117d849f0b25245

Author: Andrei Shikov

ReactAndroid/src/main/java/com/facebook/react/common/mapbuffer/ReadableMapBuffer.java

@@ -55,6 +55,7 @@ public class ReadableMapBuffer implements Iterable<ReadableMapBuffer.MapBufferEn
   @SuppressWarnings("unused")
   private short mCount = 0;
 
+  @DoNotStrip
   private ReadableMapBuffer(HybridData hybridData) {
     mHybridData = hybridData;
   }
@@ -64,23 +65,13 @@ private ReadableMapBuffer(ByteBuffer buffer) {
     readHeader();
   }
 
-  private native ByteBuffer importByteBufferAllocateDirect();
-
   private native ByteBuffer importByteBuffer();
 
   @SuppressWarnings("unused")
   @DoNotStrip
   @Nullable
   private HybridData mHybridData;
 
-  @Override
-  protected void finalize() throws Throwable {
-    super.finalize();
-    if (mHybridData != null) {
-      mHybridData.resetNative();
-    }
-  }
-
   private int getKeyOffsetForBucketIndex(int bucketIndex) {
     return HEADER_SIZE + BUCKET_SIZE * bucketIndex;
   }

ReactAndroid/src/main/java/com/facebook/react/common/mapbuffer/jni/react/common/mapbuffer/ReadableMapBuffer.cpp

@@ -13,36 +13,10 @@ namespace react {
 void ReadableMapBuffer::registerNatives() {
   registerHybrid({
       makeNativeMethod("importByteBuffer", ReadableMapBuffer::importByteBuffer),
-      makeNativeMethod(
-          "importByteBufferAllocateDirect",
-          ReadableMapBuffer::importByteBufferAllocateDirect),
   });
 }
 
-jni::local_ref<jni::JByteBuffer>
-ReadableMapBuffer::importByteBufferAllocateDirect() {
-  // TODO T83483191: Using this method is safer than "importByteBuffer" because
-  // ByteBuffer memory will be deallocated once the "Java ByteBuffer" is
-  // deallocated. Next steps:
-  // - Validate perf of this method vs importByteBuffer
-  // - Validate that there's no leaking of memory
-  react_native_assert(
-      (serializedData_ != nullptr && serializedDataSize_ != 0) &&
-      "Error serializedData_ is not initialized");
-  auto ret = jni::JByteBuffer::allocateDirect(serializedDataSize_);
-  // TODO T83483191: avoid allocating serializedData_ when using
-  // JByteBuffer::allocateDirect
-  std::memcpy(
-      ret->getDirectBytes(), (void *)serializedData_, serializedDataSize_);
-
-  // Deallocate serializedData_ since it's not necessary anymore
-  delete[] serializedData_;
-  serializedData_ = nullptr;
-  serializedDataSize_ = 0;
-  return ret;
-}
-
-jni::JByteBuffer::javaobject ReadableMapBuffer::importByteBuffer() {
+jni::local_ref<jni::JByteBuffer> ReadableMapBuffer::importByteBuffer() {
   // TODO T83483191: Reevaluate what's the best approach here (allocateDirect vs
   // DirectByteBuffer).
   //
@@ -55,21 +29,19 @@ jni::JByteBuffer::javaobject ReadableMapBuffer::importByteBuffer() {
   // - Add flags to describe if the data was already 'imported'
   // - Long-term: Consider creating a big ByteBuffer that can be re-used to
   // transfer data of multitple Maps
-  return static_cast<jni::JByteBuffer::javaobject>(
-      jni::Environment::current()->NewDirectByteBuffer(
-          (void *)serializedData_, serializedDataSize_));
+  return jni::JByteBuffer::wrapBytes(
+      serializedData_.data(), serializedData_.size());
 }
 
 jni::local_ref<ReadableMapBuffer::jhybridobject>
 ReadableMapBuffer::createWithContents(MapBuffer &&map) {
   return newObjectCxxArgs(std::move(map));
 }
 
-ReadableMapBuffer::~ReadableMapBuffer() {
-  if (serializedData_ != nullptr) {
-    delete[] serializedData_;
-    serializedData_ = nullptr;
-  }
+ReadableMapBuffer::ReadableMapBuffer(MapBuffer &&map)
+    : serializedData_(std::move(map.bytes_)) {
+  react_native_assert(
+      (serializedData_.size() != 0) && "Error no content in map");
 }
 
 } // namespace react

ReactAndroid/src/main/java/com/facebook/react/common/mapbuffer/jni/react/common/mapbuffer/ReadableMapBuffer.h

@@ -23,28 +23,15 @@ class ReadableMapBuffer : public jni::HybridClass<ReadableMapBuffer> {
 
   static void registerNatives();
 
-  static jni::local_ref<jhybridobject> createWithContents(MapBuffer &&map);
+  static jni::local_ref<ReadableMapBuffer::jhybridobject> createWithContents(
+      MapBuffer &&map);
 
-  jni::local_ref<jni::JByteBuffer> importByteBufferAllocateDirect();
+  explicit ReadableMapBuffer(MapBuffer &&map);
 
-  jni::JByteBuffer::javaobject importByteBuffer();
-
-  ~ReadableMapBuffer();
+  jni::local_ref<jni::JByteBuffer> importByteBuffer();
 
  private:
-  uint8_t *serializedData_ = nullptr;
-
-  int32_t serializedDataSize_ = 0;
-
-  friend HybridBase;
-
-  explicit ReadableMapBuffer(MapBuffer &&map) {
-    serializedDataSize_ = map.getBufferSize();
-    react_native_assert(
-        (serializedDataSize_ != 0) && "Error no content in map");
-    serializedData_ = new Byte[serializedDataSize_];
-    map.copy(serializedData_);
-  }
+  std::vector<uint8_t> serializedData_;
 };
 
 } // namespace react

ReactCommon/react/renderer/mapbuffer/MapBuffer.h

@@ -15,6 +15,8 @@
 namespace facebook {
 namespace react {
 
+class ReadableMapBuffer;
+
 /**
  * MapBuffer is an optimized map format for transferring data like props between
  * C++ and other platforms The implementation of this map is optimized to:
@@ -31,6 +33,8 @@ namespace react {
  * - have minimal APK size and build time impact.
  */
 class MapBuffer {
+  friend ReadableMapBuffer;
+
  private:
   // Buffer and its size
   std::vector<uint8_t> const bytes_;