Efficient Conversion of a FlatBufferBuilder to a MessageBuilder (#4980)

* Efficient conversion of FlatBufferBuilder to grpc::MessageBuilder

* Added a variety of tests to validate correctness of the MessageBuilder move operations.
Disable MessageBuilder half-n-half tests on MacOS.

* Fix failing Android build

* Generalized the MessageBuilder move constructor to accept a deallocator

2 files changed, 245 insertions, 82 deletions

diff --git a/grpc/tests/grpctest.cpp b/grpc/tests/grpctest.cpp
index 50b17cc0..7e5c6e6c 100644
--- a/grpc/tests/grpctest.cpp
+++ b/grpc/tests/grpctest.cpp
@@ -23,6 +23,9 @@
 #include "test_assert.h"
 
 using namespace MyGame::Example;
+using flatbuffers::grpc::MessageBuilder;
+using flatbuffers::FlatBufferBuilder;
+
 void message_builder_tests();
 
 // The callback implementation of our server, that derives from the generated
@@ -46,9 +49,9 @@ class ServiceImpl final : public MyGame::Example::MonsterStorage::Service {
       const flatbuffers::grpc::Message<Stat> *request,
       ::grpc::ServerWriter<flatbuffers::grpc::Message<Monster>> *writer)
       override {
-    for (int i = 0; i < 10; i++) {
+    for (int i = 0; i < 5; i++) {
       fbb_.Clear();
-      // Create 10 monsters for resposne.
+      // Create 5 monsters for resposne.
       auto monster_offset =
           CreateMonster(fbb_, 0, 0, 0,
                         fbb_.CreateString(request->GetRoot()->id()->str() +
@@ -94,6 +97,45 @@ void RunServer() {
   server_instance->Wait();
 }
 
+template <class Builder>
+void StoreRPC(MonsterStorage::Stub *stub) {
+  Builder fbb;
+  grpc::ClientContext context;
+  // Build a request with the name set.
+  auto monster_offset = CreateMonster(fbb, 0, 0, 0, fbb.CreateString("Fred"));
+  MessageBuilder mb(std::move(fbb));
+  mb.Finish(monster_offset);
+  auto request = mb.ReleaseMessage<Monster>();
+  flatbuffers::grpc::Message<Stat> response;
+
+  // The actual RPC.
+  auto status = stub->Store(&context, request, &response);
+
+  if (status.ok()) {
+    auto resp = response.GetRoot()->id();
+    std::cout << "RPC response: " << resp->str() << std::endl;
+  } else {
+    std::cout << "RPC failed" << std::endl;
+  }
+}
+
+template <class Builder>
+void RetrieveRPC(MonsterStorage::Stub *stub) {
+  Builder fbb;
+  grpc::ClientContext context;
+  fbb.Clear();
+  auto stat_offset = CreateStat(fbb, fbb.CreateString("Fred"));
+  fbb.Finish(stat_offset);
+  auto request = MessageBuilder(std::move(fbb)).ReleaseMessage<Stat>();
+
+  flatbuffers::grpc::Message<Monster> response;
+  auto stream = stub->Retrieve(&context, request);
+  while (stream->Read(&response)) {
+    auto resp = response.GetRoot()->name();
+    std::cout << "RPC Streaming response: " << resp->str() << std::endl;
+  }
+}
+
 int grpc_server_test() {
   // Launch server.
   std::thread server_thread(RunServer);
@@ -107,39 +149,12 @@ int grpc_server_test() {
                                      grpc::InsecureChannelCredentials());
   auto stub = MyGame::Example::MonsterStorage::NewStub(channel);
 
-  flatbuffers::grpc::MessageBuilder fbb;
-  {
-    grpc::ClientContext context;
-    // Build a request with the name set.
-    auto monster_offset = CreateMonster(fbb, 0, 0, 0, fbb.CreateString("Fred"));
-    fbb.Finish(monster_offset);
-    auto request = fbb.ReleaseMessage<Monster>();
-    flatbuffers::grpc::Message<Stat> response;
+  StoreRPC<MessageBuilder>(stub.get());
+  StoreRPC<FlatBufferBuilder>(stub.get());
 
-    // The actual RPC.
-    auto status = stub->Store(&context, request, &response);
+  RetrieveRPC<MessageBuilder>(stub.get());
+  RetrieveRPC<FlatBufferBuilder>(stub.get());
 
-    if (status.ok()) {
-      auto resp = response.GetRoot()->id();
-      std::cout << "RPC response: " << resp->str() << std::endl;
-    } else {
-      std::cout << "RPC failed" << std::endl;
-    }
-  }
-  {
-    grpc::ClientContext context;
-    fbb.Clear();
-    auto stat_offset = CreateStat(fbb, fbb.CreateString("Fred"));
-    fbb.Finish(stat_offset);
-    auto request = fbb.ReleaseMessage<Stat>();
-
-    flatbuffers::grpc::Message<Monster> response;
-    auto stream = stub->Retrieve(&context, request);
-    while (stream->Read(&response)) {
-      auto resp = response.GetRoot()->name();
-      std::cout << "RPC Streaming response: " << resp->str() << std::endl;
-    }
-  }
 
 #if !FLATBUFFERS_GRPC_DISABLE_AUTO_VERIFICATION
   {
diff --git a/grpc/tests/message_builder_test.cpp b/grpc/tests/message_builder_test.cpp
index 25f04da8..d5ed8d97 100644
--- a/grpc/tests/message_builder_test.cpp
+++ b/grpc/tests/message_builder_test.cpp
@@ -14,19 +14,37 @@ bool release_n_verify(flatbuffers::grpc::MessageBuilder &mbb, const std::string
   return (monster->name()->str() == expected_name) && (monster->color() == color);
 }
 
-template <>
-struct BuilderReuseTests<flatbuffers::grpc::MessageBuilder> {
+void builder_move_assign_after_releaseraw_test(flatbuffers::grpc::MessageBuilder dst) {
+  auto root_offset1 = populate1(dst);
+  dst.Finish(root_offset1);
+  size_t size, offset;
+  grpc_slice slice;
+  dst.ReleaseRaw(size, offset, slice);
+  flatbuffers::FlatBufferBuilder src;
+  auto root_offset2 = populate2(src);
+  src.Finish(root_offset2);
+  auto src_size = src.GetSize();
+  // Move into a released builder.
+  dst = std::move(src);
+  TEST_EQ(dst.GetSize(), src_size);
+  TEST_ASSERT(release_n_verify(dst, m2_name, m2_color));
+  TEST_EQ(src.GetSize(), 0);
+  grpc_slice_unref(slice);
+}
+
+template <class SrcBuilder>
+struct BuilderReuseTests<flatbuffers::grpc::MessageBuilder, SrcBuilder> {
   static void builder_reusable_after_release_message_test(TestSelector selector) {
     if (!selector.count(REUSABLE_AFTER_RELEASE_MESSAGE)) {
       return;
     }
 
-    flatbuffers::grpc::MessageBuilder b1;
+    flatbuffers::grpc::MessageBuilder mb;
     std::vector<flatbuffers::grpc::Message<Monster>> buffers;
     for (int i = 0; i < 5; ++i) {
-      auto root_offset1 = populate1(b1);
-      b1.Finish(root_offset1);
-      buffers.push_back(b1.ReleaseMessage<Monster>());
+      auto root_offset1 = populate1(mb);
+      mb.Finish(root_offset1);
+      buffers.push_back(mb.ReleaseMessage<Monster>());
       TEST_ASSERT_FUNC(verify(buffers[i], m1_name, m1_color));
     }
   }
@@ -36,14 +54,15 @@ struct BuilderReuseTests<flatbuffers::grpc::MessageBuilder> {
       return;
     }
 
-    // FIXME: Populate-Release loop fails assert(GRPC_SLICE_IS_EMPTY(slice_)).
+    // FIXME: Populate-Release loop fails assert(GRPC_SLICE_IS_EMPTY(slice_)) in SliceAllocator::allocate
+    // in the second iteration.
 
-    flatbuffers::grpc::MessageBuilder b1;
+    flatbuffers::grpc::MessageBuilder mb;
     std::vector<flatbuffers::DetachedBuffer> buffers;
-    for (int i = 0; i < 5; ++i) {
-      auto root_offset1 = populate1(b1);
-      b1.Finish(root_offset1);
-      buffers.push_back(b1.Release());
+    for (int i = 0; i < 2; ++i) {
+      auto root_offset1 = populate1(mb);
+      mb.Finish(root_offset1);
+      buffers.push_back(mb.Release());
       TEST_ASSERT_FUNC(verify(buffers[i], m1_name, m1_color));
     }
   }
@@ -53,13 +72,13 @@ struct BuilderReuseTests<flatbuffers::grpc::MessageBuilder> {
       return;
     }
 
-    flatbuffers::grpc::MessageBuilder b1;
+    flatbuffers::grpc::MessageBuilder mb;
     for (int i = 0; i < 5; ++i) {
-      auto root_offset1 = populate1(b1);
-      b1.Finish(root_offset1);
+      auto root_offset1 = populate1(mb);
+      mb.Finish(root_offset1);
       size_t size, offset;
       grpc_slice slice;
-      const uint8_t *buf = b1.ReleaseRaw(size, offset, slice);
+      const uint8_t *buf = mb.ReleaseRaw(size, offset, slice);
       TEST_ASSERT_FUNC(verify(buf, offset, m1_name, m1_color));
       grpc_slice_unref(slice);
     }
@@ -70,22 +89,23 @@ struct BuilderReuseTests<flatbuffers::grpc::MessageBuilder> {
       return;
     }
 
-    // FIXME: Release-move_assign loop fails assert(p == GRPC_SLICE_START_PTR(slice_)).
+    // FIXME: Release-move_assign loop fails assert(p == GRPC_SLICE_START_PTR(slice_))
+    // in DetachedBuffer destructor after all the iterations
 
-    flatbuffers::grpc::MessageBuilder b1;
+    flatbuffers::grpc::MessageBuilder dst;
     std::vector<flatbuffers::DetachedBuffer> buffers;
 
-    for (int i = 0; i < 1; ++i) {
-      auto root_offset1 = populate1(b1);
-      b1.Finish(root_offset1);
-      buffers.push_back(b1.Release());
+    for (int i = 0; i < 2; ++i) {
+      auto root_offset1 = populate1(dst);
+      dst.Finish(root_offset1);
+      buffers.push_back(dst.Release());
       TEST_ASSERT_FUNC(verify(buffers[i], m1_name, m1_color));
 
-      // bring b1 back to life.
-      flatbuffers::grpc::MessageBuilder b2;
-      b1 = std::move(b2);
-      TEST_EQ_FUNC(b1.GetSize(), 0);
-      TEST_EQ_FUNC(b2.GetSize(), 0);
+      // bring dst back to life.
+      SrcBuilder src;
+      dst = std::move(src);
+      TEST_EQ_FUNC(dst.GetSize(), 0);
+      TEST_EQ_FUNC(src.GetSize(), 0);
     }
   }
 
@@ -94,20 +114,20 @@ struct BuilderReuseTests<flatbuffers::grpc::MessageBuilder> {
       return;
     }
 
-    flatbuffers::grpc::MessageBuilder b1;
+    flatbuffers::grpc::MessageBuilder dst;
     std::vector<flatbuffers::grpc::Message<Monster>> buffers;
 
     for (int i = 0; i < 5; ++i) {
-      auto root_offset1 = populate1(b1);
-      b1.Finish(root_offset1);
-      buffers.push_back(b1.ReleaseMessage<Monster>());
+      auto root_offset1 = populate1(dst);
+      dst.Finish(root_offset1);
+      buffers.push_back(dst.ReleaseMessage<Monster>());
       TEST_ASSERT_FUNC(verify(buffers[i], m1_name, m1_color));
 
-      // bring b1 back to life.
-      flatbuffers::grpc::MessageBuilder b2;
-      b1 = std::move(b2);
-      TEST_EQ_FUNC(b1.GetSize(), 0);
-      TEST_EQ_FUNC(b2.GetSize(), 0);
+      // bring dst back to life.
+      SrcBuilder src;
+      dst = std::move(src);
+      TEST_EQ_FUNC(dst.GetSize(), 0);
+      TEST_EQ_FUNC(src.GetSize(), 0);
     }
   }
 
@@ -116,20 +136,20 @@ struct BuilderReuseTests<flatbuffers::grpc::MessageBuilder> {
       return;
     }
 
-    flatbuffers::grpc::MessageBuilder b1;
+    flatbuffers::grpc::MessageBuilder dst;
     for (int i = 0; i < 5; ++i) {
-      auto root_offset1 = populate1(b1);
-      b1.Finish(root_offset1);
+      auto root_offset1 = populate1(dst);
+      dst.Finish(root_offset1);
       size_t size, offset;
       grpc_slice slice = grpc_empty_slice();
-      const uint8_t *buf = b1.ReleaseRaw(size, offset, slice);
+      const uint8_t *buf = dst.ReleaseRaw(size, offset, slice);
       TEST_ASSERT_FUNC(verify(buf, offset, m1_name, m1_color));
       grpc_slice_unref(slice);
 
-      flatbuffers::grpc::MessageBuilder b2;
-      b1 = std::move(b2); 
-      TEST_EQ_FUNC(b1.GetSize(), 0);
-      TEST_EQ_FUNC(b2.GetSize(), 0);
+      SrcBuilder src;
+      dst = std::move(src);
+      TEST_EQ_FUNC(dst.GetSize(), 0);
+      TEST_EQ_FUNC(src.GetSize(), 0);
     }
   }
 
@@ -153,7 +173,7 @@ void slice_allocator_tests() {
     buf[0] = 100;
     buf[size-1] = 200;
     flatbuffers::grpc::SliceAllocator sa2(std::move(sa1));
-    // buf should be deleted after move-construct
+    // buf should not be deleted after move-construct
     TEST_EQ_FUNC(buf[0], 100);
     TEST_EQ_FUNC(buf[size-1], 200);
     // buf is freed here
@@ -170,13 +190,140 @@ void slice_allocator_tests() {
   }
 }
 
+/// This function does not populate exactly the first half of the table. But it could.
+void populate_first_half(MyGame::Example::MonsterBuilder &wrapper, flatbuffers::Offset<flatbuffers::String> name_offset) {
+  wrapper.add_name(name_offset);
+  wrapper.add_color(m1_color);
+}
+
+/// This function does not populate exactly the second half of the table. But it could.
+void populate_second_half(MyGame::Example::MonsterBuilder &wrapper) {
+  wrapper.add_hp(77);
+  wrapper.add_mana(88);
+  Vec3 vec3;
+  wrapper.add_pos(&vec3);
+}
+
+/// This function is a hack to update the FlatBufferBuilder reference (fbb_) in the MonsterBuilder object.
+/// This function will break if fbb_ is not the first member in MonsterBuilder. In that case, some offset must be added.
+/// This function is used exclusively for testing correctness of move operations between FlatBufferBuilders.
+/// If MonsterBuilder had a fbb_ pointer, this hack would be unnecessary. That involves a code-generator change though.
+void test_only_hack_update_fbb_reference(MyGame::Example::MonsterBuilder &monsterBuilder,
+                                         flatbuffers::grpc::MessageBuilder &mb) {
+  *reinterpret_cast<flatbuffers::FlatBufferBuilder **>(&monsterBuilder) = &mb;
+}
+
+/// This test validates correctness of move conversion of FlatBufferBuilder to a MessageBuilder DURING
+/// a table construction. Half of the table is constructed using FlatBufferBuilder and the other half
+/// of the table is constructed using a MessageBuilder.
+void builder_move_ctor_conversion_before_finish_half_n_half_table_test() {
+  for (size_t initial_size = 4 ; initial_size <= 2048; initial_size *= 2) {
+    flatbuffers::FlatBufferBuilder fbb(initial_size);
+    auto name_offset = fbb.CreateString(m1_name);
+    MyGame::Example::MonsterBuilder monsterBuilder(fbb);     // starts a table in FlatBufferBuilder
+    populate_first_half(monsterBuilder, name_offset);
+    flatbuffers::grpc::MessageBuilder mb(std::move(fbb));
+    test_only_hack_update_fbb_reference(monsterBuilder, mb); // hack
+    populate_second_half(monsterBuilder);
+    mb.Finish(monsterBuilder.Finish());                      // ends the table in MessageBuilder
+    TEST_ASSERT_FUNC(release_n_verify(mb, m1_name, m1_color));
+    TEST_EQ_FUNC(fbb.GetSize(), 0);
+  }
+}
+
+/// This test populates a COMPLETE inner table before move conversion and later populates more members in the outer table.
+void builder_move_ctor_conversion_before_finish_test() {
+  for (size_t initial_size = 4 ; initial_size <= 2048; initial_size *= 2) {
+    flatbuffers::FlatBufferBuilder fbb(initial_size);
+    auto stat_offset = CreateStat(fbb, fbb.CreateString("SomeId"), 0, 0);
+    flatbuffers::grpc::MessageBuilder mb(std::move(fbb));
+    auto monster_offset = CreateMonster(mb, 0, 150, 100, mb.CreateString(m1_name), 0, m1_color, Any_NONE, 0, 0, 0, 0, 0, 0, stat_offset);
+    mb.Finish(monster_offset);
+    TEST_ASSERT_FUNC(release_n_verify(mb, m1_name, m1_color));
+    TEST_EQ_FUNC(fbb.GetSize(), 0);
+  }
+}
+
+/// This test validates correctness of move conversion of FlatBufferBuilder to a MessageBuilder DURING
+/// a table construction. Half of the table is constructed using FlatBufferBuilder and the other half
+/// of the table is constructed using a MessageBuilder.
+void builder_move_assign_conversion_before_finish_half_n_half_table_test() {
+  flatbuffers::FlatBufferBuilder fbb;
+  flatbuffers::grpc::MessageBuilder mb;
+
+  for (int i = 0;i < 5; ++i) {
+    flatbuffers::FlatBufferBuilder fbb;
+    auto name_offset = fbb.CreateString(m1_name);
+    MyGame::Example::MonsterBuilder monsterBuilder(fbb);     // starts a table in FlatBufferBuilder
+    populate_first_half(monsterBuilder, name_offset);
+    mb = std::move(fbb);
+    test_only_hack_update_fbb_reference(monsterBuilder, mb); // hack
+    populate_second_half(monsterBuilder);
+    mb.Finish(monsterBuilder.Finish());                      // ends the table in MessageBuilder
+    TEST_ASSERT_FUNC(release_n_verify(mb, m1_name, m1_color));
+    TEST_EQ_FUNC(fbb.GetSize(), 0);
+  }
+}
+
+/// This test populates a COMPLETE inner table before move conversion and later populates more members in the outer table.
+void builder_move_assign_conversion_before_finish_test() {
+  flatbuffers::FlatBufferBuilder fbb;
+  flatbuffers::grpc::MessageBuilder mb;
+
+  for (int i = 0;i < 5; ++i) {
+    auto stat_offset = CreateStat(fbb, fbb.CreateString("SomeId"), 0, 0);
+    mb = std::move(fbb);
+    auto monster_offset = CreateMonster(mb, 0, 150, 100, mb.CreateString(m1_name), 0, m1_color, Any_NONE, 0, 0, 0, 0, 0, 0, stat_offset);
+    mb.Finish(monster_offset);
+    TEST_ASSERT_FUNC(release_n_verify(mb, m1_name, m1_color));
+    TEST_EQ_FUNC(fbb.GetSize(), 0);
+  }
+}
+
+/// This test populates data, finishes the buffer, and does move conversion after.
+void builder_move_ctor_conversion_after_finish_test() {
+  flatbuffers::FlatBufferBuilder fbb;
+  fbb.Finish(populate1(fbb));
+  flatbuffers::grpc::MessageBuilder mb(std::move(fbb));
+  TEST_ASSERT_FUNC(release_n_verify(mb, m1_name, m1_color));
+  TEST_EQ_FUNC(fbb.GetSize(), 0);
+}
+
+/// This test populates data, finishes the buffer, and does move conversion after.
+void builder_move_assign_conversion_after_finish_test() {
+  flatbuffers::FlatBufferBuilder fbb;
+  flatbuffers::grpc::MessageBuilder mb;
+
+  for (int i = 0;i < 5; ++i) {
+    fbb.Finish(populate1(fbb));
+    mb = std::move(fbb);
+    TEST_ASSERT_FUNC(release_n_verify(mb, m1_name, m1_color));
+    TEST_EQ_FUNC(fbb.GetSize(), 0);
+  }
+}
+
 void message_builder_tests() {
+  using flatbuffers::grpc::MessageBuilder;
+  using flatbuffers::FlatBufferBuilder;
+
   slice_allocator_tests();
-  BuilderTests<flatbuffers::grpc::MessageBuilder>::all_tests();
+
+#ifndef __APPLE__
+  builder_move_ctor_conversion_before_finish_half_n_half_table_test();
+  builder_move_assign_conversion_before_finish_half_n_half_table_test();
+#endif // __APPLE__
+  builder_move_ctor_conversion_before_finish_test();
+  builder_move_assign_conversion_before_finish_test();
+
+  builder_move_ctor_conversion_after_finish_test();
+  builder_move_assign_conversion_after_finish_test();
+
+  BuilderTests<MessageBuilder, MessageBuilder>::all_tests();
+  BuilderTests<MessageBuilder, FlatBufferBuilder>::all_tests();
 
   BuilderReuseTestSelector tests[6] = {
-    // REUSABLE_AFTER_RELEASE,                 // Assertion failed: (GRPC_SLICE_IS_EMPTY(slice_))
-    // REUSABLE_AFTER_RELEASE_AND_MOVE_ASSIGN, // Assertion failed: (p == GRPC_SLICE_START_PTR(slice_)
+    //REUSABLE_AFTER_RELEASE,                 // Assertion failed: (GRPC_SLICE_IS_EMPTY(slice_))
+    //REUSABLE_AFTER_RELEASE_AND_MOVE_ASSIGN, // Assertion failed: (p == GRPC_SLICE_START_PTR(slice_)
 
     REUSABLE_AFTER_RELEASE_RAW,
     REUSABLE_AFTER_RELEASE_MESSAGE,
@@ -184,5 +331,6 @@ void message_builder_tests() {
     REUSABLE_AFTER_RELEASE_RAW_AND_MOVE_ASSIGN
   };
 
-  BuilderReuseTests<flatbuffers::grpc::MessageBuilder>::run_tests(TestSelector(tests, tests+6));
+  BuilderReuseTests<MessageBuilder, MessageBuilder>::run_tests(TestSelector(tests, tests+6));
+  BuilderReuseTests<MessageBuilder, FlatBufferBuilder>::run_tests(TestSelector(tests, tests+6));
 }