From e7578548a5714dd278d798344d6619d8cbbfb4d9 Mon Sep 17 00:00:00 2001 From: Sumant Tambe Date: Thu, 30 Aug 2018 16:43:22 -0700 Subject: [PATCH] Add move semantics to MessageBuilder, FlatBufferBuilder, SliceAllocator, and vector_downward (#4893) Unit tests Update flatbuffers + gRPC build instructions Update CMakeLists.txt with cmake variables for grpc and protobuf install paths Update tests for travis build --- CMakeLists.txt | 13 +++ grpc/README.md | 20 ++++ grpc/tests/grpctest.cpp | 8 +- grpc/tests/message_builder_test.cpp | 159 ++++++++++++++++++++++++++++ include/flatbuffers/flatbuffers.h | 82 ++++++++++++++ include/flatbuffers/grpc.h | 39 +++++++ 6 files changed, 320 insertions(+), 1 deletion(-) create mode 100644 grpc/tests/message_builder_test.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index af1048dbe..dab15cafd 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -101,6 +101,7 @@ set(FlatBuffers_GRPCTest_SRCS tests/monster_test.grpc.fb.h tests/monster_test.grpc.fb.cc grpc/tests/grpctest.cpp + grpc/tests/message_builder_test.cpp # file generated by running compiler on samples/monster.fbs ${CMAKE_CURRENT_BINARY_DIR}/samples/monster_generated.h ) @@ -257,6 +258,15 @@ if(FLATBUFFERS_BUILD_GRPCTEST) if(CMAKE_COMPILER_IS_GNUCXX) set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unused-parameter -Wno-shadow") endif() + if(NOT GRPC_INSTALL_PATH) + message(SEND_ERROR "GRPC_INSTALL_PATH variable is not defined. See grpc/README.md") + endif() + if(NOT PROTOBUF_DOWNLOAD_PATH) + message(SEND_ERROR "PROTOBUF_DOWNLOAD_PATH variable is not defined. See grpc/README.md") + endif() + INCLUDE_DIRECTORIES(${GRPC_INSTALL_PATH}/include) + INCLUDE_DIRECTORIES(${PROTOBUF_DOWNLOAD_PATH}/src) + LINK_DIRECTORIES(${GRPC_INSTALL_PATH}/lib) add_executable(grpctest ${FlatBuffers_GRPCTest_SRCS}) target_link_libraries(grpctest grpc++_unsecure grpc_unsecure gpr pthread dl) endif() @@ -346,6 +356,9 @@ if(FLATBUFFERS_BUILD_TESTS) file(COPY "${CMAKE_CURRENT_SOURCE_DIR}/tests" DESTINATION "${CMAKE_CURRENT_BINARY_DIR}") add_test(NAME flattests COMMAND flattests) + if(FLATBUFFERS_BUILD_GRPCTEST) + add_test(NAME grpctest COMMAND grpctest) + endif() endif() include(CMake/BuildFlatBuffers.cmake) diff --git a/grpc/README.md b/grpc/README.md index 13485198c..544651d60 100644 --- a/grpc/README.md +++ b/grpc/README.md @@ -9,3 +9,23 @@ from GRPC, and work with both the Protobuf and FlatBuffers code generator. the GRPC libraries for this to compile. This test will build using the `FLATBUFFERS_BUILD_GRPCTEST` option to the main FlatBuffers CMake project. +## Building Flatbuffers with gRPC + +### Linux + +1. Download, build and install gRPC. See [instructions](https://github.com/grpc/grpc/tree/master/src/cpp). + * Lets say your gRPC clone is at `/your/path/to/grpc_repo`. + * Install gRPC in a custom directory by running `make install prefix=/your/path/to/grpc_repo/install`. +2. `export GRPC_INSTALL_PATH=/your/path/to/grpc_repo/install` +3. `export PROTOBUF_DOWNLOAD_PATH=/your/path/to/grpc_repo/third_party/protobuf` +4. `mkdir build ; cd build` +5. `cmake -DFLATBUFFERS_BUILD_GRPCTEST=ON -DGRPC_INSTALL_PATH=${GRPC_INSTALL_PATH} -DPROTOBUF_DOWNLOAD_PATH=${PROTOBUF_DOWNLOAD_PATH} ..` +6. `make` + +## Running FlatBuffer gRPC tests + +### Linux + +1. `ln -s ${GRPC_INSTALL_PATH}/lib/libgrpc++_unsecure.so.6 ${GRPC_INSTALL_PATH}/lib/libgrpc++_unsecure.so.1` +2. `export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:${GRPC_INSTALL_PATH}/lib` +3. `make test ARGS=-V` diff --git a/grpc/tests/grpctest.cpp b/grpc/tests/grpctest.cpp index cbb9bac1b..cb4326ecd 100644 --- a/grpc/tests/grpctest.cpp +++ b/grpc/tests/grpctest.cpp @@ -22,6 +22,7 @@ #include "monster_test_generated.h" using namespace MyGame::Example; +int builder_tests(); // The callback implementation of our server, that derives from the generated // code. It implements all rpcs specified in the FlatBuffers schema. @@ -92,7 +93,7 @@ void RunServer() { server_instance->Wait(); } -int main(int /*argc*/, const char * /*argv*/ []) { +int grpc_server_test() { // Launch server. std::thread server_thread(RunServer); @@ -163,3 +164,8 @@ int main(int /*argc*/, const char * /*argv*/ []) { return 0; } + +int main(int /*argc*/, const char * /*argv*/ []) { + return builder_tests() + grpc_server_test(); +} + diff --git a/grpc/tests/message_builder_test.cpp b/grpc/tests/message_builder_test.cpp new file mode 100644 index 000000000..974e050c9 --- /dev/null +++ b/grpc/tests/message_builder_test.cpp @@ -0,0 +1,159 @@ +#include "flatbuffers/grpc.h" +#include "monster_test_generated.h" + +static int builder_test_error = 0; + +#define test_assert(condition) do { \ + if(!(condition)) { \ + fprintf(stderr, "%s:%d: %s failed.\n", __FILE__, __LINE__, #condition);\ + builder_test_error = 1;\ + } \ +} while(0) + +using namespace MyGame::Example; + +const std::string m1_name = "Cyberdemon"; +const Color m1_color = Color_Red; +const std::string m2_name = "Imp"; +const Color m2_color = Color_Green; + +flatbuffers::Offset populate1(flatbuffers::FlatBufferBuilder &builder) { + auto name_offset = builder.CreateString(m1_name); + return CreateMonster(builder, nullptr, 0, 0, name_offset, 0, m1_color); +} + +flatbuffers::Offset populate2(flatbuffers::FlatBufferBuilder &builder) { + auto name_offset = builder.CreateString(m2_name); + return CreateMonster(builder, nullptr, 0, 0, name_offset, 0, m2_color); +} + +bool release_n_verify(flatbuffers::FlatBufferBuilder &fbb, const std::string &expected_name, Color color) { + flatbuffers::DetachedBuffer buf = fbb.Release(); + const Monster *monster = flatbuffers::GetRoot(buf.data()); + return (monster->name()->str() == expected_name) && (monster->color() == color); +} + +bool release_n_verify(flatbuffers::grpc::MessageBuilder &mbb, const std::string &expected_name, Color color) { + flatbuffers::grpc::Message msg = mbb.ReleaseMessage(); + const Monster *monster = msg.GetRoot(); + return (monster->name()->str() == expected_name) && (monster->color() == color); +} + +struct OwnedAllocator : public flatbuffers::DefaultAllocator {}; + +struct TestHeapMessageBuilder : public flatbuffers::FlatBufferBuilder { + TestHeapMessageBuilder() + : flatbuffers::FlatBufferBuilder(2048, new OwnedAllocator(), true) {} +}; + +template +struct BuilderTests { + static void empty_builder_movector_test() { + Builder b1; + size_t b1_size = b1.GetSize(); + Builder b2(std::move(b1)); + size_t b2_size = b2.GetSize(); + test_assert(b1_size == 0); + test_assert(b1_size == b2_size); + } + + static void nonempty_builder_movector_test() { + Builder b1; + populate1(b1); + size_t b1_size = b1.GetSize(); + Builder b2(std::move(b1)); + test_assert(b1_size == b2.GetSize()); + test_assert(0 == b1.GetSize()); + } + + static void builder_movector_before_finish_test() { + Builder b1; + auto root_offset1 = populate1(b1); + Builder b2(std::move(b1)); + b2.Finish(root_offset1); + test_assert(release_n_verify(b2, m1_name, m1_color)); + test_assert(0 == b1.GetSize()); + } + + static void builder_movector_after_finish_test() { + Builder b1; + auto root_offset1 = populate1(b1); + b1.Finish(root_offset1); + Builder b2(std::move(b1)); + test_assert(release_n_verify(b2, m1_name, m1_color)); + test_assert(0 == b1.GetSize()); + } + + static void builder_move_assign_before_finish_test() { + Builder b1; + auto root_offset1 = populate1(b1); + Builder b2; + populate2(b2); + b2 = std::move(b1); + b2.Finish(root_offset1); + test_assert(release_n_verify(b2, m1_name, m1_color)); + test_assert(0 == b1.GetSize()); + } + + static void builder_move_assign_after_finish_test() { + Builder b1; + auto root_offset1 = populate1(b1); + b1.Finish(root_offset1); + Builder b2; + auto root_offset2 = populate2(b2); + b2.Finish(root_offset2); + b2 = std::move(b1); + test_assert(release_n_verify(b2, m1_name, m1_color)); + test_assert(0 == b1.GetSize()); + } + + static void builder_swap_before_finish_test() { + Builder b1; + auto root_offset1 = populate1(b1); + auto size1 = b1.GetSize(); + Builder b2; + auto root_offset2 = populate2(b2); + auto size2 = b2.GetSize(); + b1.Swap(b2); + b1.Finish(root_offset2); + b2.Finish(root_offset1); + test_assert(b1.GetSize() > size2); + test_assert(b2.GetSize() > size1); + test_assert(release_n_verify(b1, m2_name, m2_color)); + test_assert(release_n_verify(b2, m1_name, m1_color)); + } + + static void builder_swap_after_finish_test() { + Builder b1; + auto root_offset1 = populate1(b1); + b1.Finish(root_offset1); + auto size1 = b1.GetSize(); + Builder b2; + auto root_offset2 = populate2(b2); + b2.Finish(root_offset2); + auto size2 = b2.GetSize(); + b1.Swap(b2); + test_assert(b1.GetSize() == size2); + test_assert(b2.GetSize() == size1); + test_assert(release_n_verify(b1, m2_name, m2_color)); + test_assert(release_n_verify(b2, m1_name, m1_color)); + } + + static void all_tests() { + empty_builder_movector_test(); + nonempty_builder_movector_test(); + builder_movector_before_finish_test(); + builder_movector_after_finish_test(); + builder_move_assign_before_finish_test(); + builder_move_assign_after_finish_test(); + builder_swap_before_finish_test(); + builder_swap_after_finish_test(); + } +}; + +int builder_tests() { + BuilderTests::all_tests(); + BuilderTests::all_tests(); + BuilderTests::all_tests(); + return builder_test_error; +} diff --git a/include/flatbuffers/flatbuffers.h b/include/flatbuffers/flatbuffers.h index d0b96abc0..386f2cfef 100644 --- a/include/flatbuffers/flatbuffers.h +++ b/include/flatbuffers/flatbuffers.h @@ -572,6 +572,32 @@ class vector_downward { cur_(nullptr), scratch_(nullptr) {} + vector_downward(vector_downward &&other) + : allocator_(other.allocator_), + own_allocator_(other.own_allocator_), + initial_size_(other.initial_size_), + buffer_minalign_(other.buffer_minalign_), + reserved_(other.reserved_), + buf_(other.buf_), + cur_(other.cur_), + scratch_(other.scratch_) { + other.allocator_ = nullptr; + other.own_allocator_ = false; + // No change in other.initial_size_ + // No change in other.buffer_minalign_ + other.reserved_ = 0; + other.buf_ = nullptr; + other.cur_ = nullptr; + other.scratch_ = nullptr; + } + + vector_downward &operator=(vector_downward &&other) { + // Move construct a temporary and swap idiom + vector_downward temp(std::move(other)); + swap(temp); + return *this; + } + ~vector_downward() { clear_buffer(); clear_allocator(); @@ -706,6 +732,24 @@ class vector_downward { void pop(size_t bytes_to_remove) { cur_ += bytes_to_remove; } void scratch_pop(size_t bytes_to_remove) { scratch_ -= bytes_to_remove; } + void swap(vector_downward &other) { + using std::swap; + swap(allocator_, other.allocator_); + swap(own_allocator_, other.own_allocator_); + swap(initial_size_, other.initial_size_); + swap(buffer_minalign_, other.buffer_minalign_); + swap(reserved_, other.reserved_); + swap(buf_, other.buf_); + swap(cur_, other.cur_); + swap(scratch_, other.scratch_); + } + + void swap_allocator(vector_downward &other) { + using std::swap; + swap(allocator_, other.allocator_); + swap(own_allocator_, other.own_allocator_); + } + private: // You shouldn't really be copying instances of this class. FLATBUFFERS_DELETE_FUNC(vector_downward(const vector_downward &)) @@ -794,6 +838,44 @@ class FlatBufferBuilder { EndianCheck(); } + /// @brief Move constructor for FlatBufferBuilder. + FlatBufferBuilder(FlatBufferBuilder &&other) + : buf_(1024, nullptr, false, AlignOf()), + num_field_loc(0), + max_voffset_(0), + nested(false), + finished(false), + minalign_(1), + force_defaults_(false), + dedup_vtables_(true), + string_pool(nullptr) { + EndianCheck(); + // Default construct and swap idiom. + // Lack of delegating constructors in vs2010 makes it more verbose than needed. + Swap(other); + } + + /// @brief Move assignment operator for FlatBufferBuilder. + FlatBufferBuilder &operator=(FlatBufferBuilder &&other) { + // Move construct a temporary and swap idiom + FlatBufferBuilder temp(std::move(other)); + Swap(temp); + return *this; + } + + void Swap(FlatBufferBuilder &other) { + using std::swap; + buf_.swap(other.buf_); + swap(num_field_loc, other.num_field_loc); + swap(max_voffset_, other.max_voffset_); + swap(nested, other.nested); + swap(finished, other.finished); + swap(minalign_, other.minalign_); + swap(force_defaults_, other.force_defaults_); + swap(dedup_vtables_, other.dedup_vtables_); + swap(string_pool, other.string_pool); + } + ~FlatBufferBuilder() { if (string_pool) delete string_pool; } diff --git a/include/flatbuffers/grpc.h b/include/flatbuffers/grpc.h index 2c71c7da1..2bcfa44a1 100644 --- a/include/flatbuffers/grpc.h +++ b/include/flatbuffers/grpc.h @@ -88,6 +88,22 @@ class SliceAllocator : public Allocator { SliceAllocator(const SliceAllocator &other) = delete; SliceAllocator &operator=(const SliceAllocator &other) = delete; + SliceAllocator(SliceAllocator &&other) + : slice_(other.slice_) { + other.slice_ = grpc_empty_slice(); + } + + SliceAllocator &operator=(SliceAllocator &&other) { + slice_ = other.slice_; + other.slice_ = grpc_empty_slice(); + return *this; + } + + void swap(SliceAllocator &other) { + using std::swap; + swap(slice_, other.slice_); + } + virtual ~SliceAllocator() { grpc_slice_unref(slice_); } virtual uint8_t *allocate(size_t size) override { @@ -151,6 +167,29 @@ class MessageBuilder : private detail::SliceAllocatorMember, MessageBuilder(const MessageBuilder &other) = delete; MessageBuilder &operator=(const MessageBuilder &other) = delete; + MessageBuilder(MessageBuilder &&other) + : FlatBufferBuilder(1024, &slice_allocator_, false) { + // Default construct and swap idiom. + Swap(other); + } + + MessageBuilder &operator=(MessageBuilder &&other) { + // Move construct a temporary and swap + MessageBuilder temp(std::move(other)); + Swap(temp); + return *this; + } + + void Swap(MessageBuilder &other) { + slice_allocator_.swap(other.slice_allocator_); + FlatBufferBuilder::Swap(other); + // After swapping the FlatBufferBuilder, we swap back the allocator, which restores + // the original allocator back in place. This is necessary because MessageBuilder's + // allocator is its own member (SliceAllocatorMember). The allocator passed to + // FlatBufferBuilder::vector_downward must point to this member. + buf_.swap_allocator(other.buf_); + } + ~MessageBuilder() {} // GetMessage extracts the subslice of the buffer corresponding to the