From e2afeabf0d159065aff496ee7b6d5e1ff52ef473 Mon Sep 17 00:00:00 2001 From: Vivek Panyam Date: Thu, 28 May 2020 11:20:35 -0400 Subject: [PATCH] [OPE] Improve IPC serialization of structs (#362) This PR uses reflection to serialize structs instead of requiring us to define `ipc_serialize` and `ipc_deserialize` for every struct we want to pass to the worker process. This is much less error prone than the manual method since adding a field to a struct no longer requires modifying serialization code. As part of this, we switch to building Neuropod with C++17. We still build in a manylinux2014 compatible sysroot so we can't use all C++17 library features, but this lets us use some C++17 language features. --- source/.bazelrc | 5 +- source/WORKSPACE | 8 +++ source/deps/BUILD.pfr | 28 ++++++++++ .../neuropod/backends/tensorflow/tf_tensor.hh | 17 +++++- source/neuropod/multiprocess/BUILD | 1 - .../neuropod/multiprocess/ope_load_config.cc | 52 ------------------- .../neuropod/multiprocess/ope_load_config.hh | 7 --- .../neuropod/multiprocess/serialization/BUILD | 1 + .../serialization/ipc_serialization.hh | 34 ++++++++++-- 9 files changed, 85 insertions(+), 68 deletions(-) create mode 100644 source/deps/BUILD.pfr delete mode 100644 source/neuropod/multiprocess/ope_load_config.cc diff --git a/source/.bazelrc b/source/.bazelrc index 60bad347..61bb1691 100644 --- a/source/.bazelrc +++ b/source/.bazelrc @@ -7,8 +7,9 @@ build --test_env=VIRTUAL_ENV # Use an internal toolchain to isolate the build from system libraries build --crosstool_top=@llvm_toolchain//:toolchain -# Use C++14 -build --cxxopt='-std=c++14' +# Use some C++17 features. We still build in a C++14 sysroot so can't use all of them +# (Linking will fail during linux builds if you're using a C++17 feature that isn't available) +build --cxxopt='-std=c++17' # Build in a mostly-static way build --dynamic_mode off diff --git a/source/WORKSPACE b/source/WORKSPACE index a2a34a72..53fa546d 100644 --- a/source/WORKSPACE +++ b/source/WORKSPACE @@ -183,3 +183,11 @@ http_archive( strip_prefix = "cpp-semver-ff341259099465283bc3dc60fb5ede16776e1607", url = "https://github.com/easz/cpp-semver/archive/ff341259099465283bc3dc60fb5ede16776e1607.zip", ) + +http_archive( + name = "pfr_repo", + build_file = "@//deps:BUILD.pfr", + sha256 = "92e0ffa3be49ffb21486e8e929dde599455fc1818054254bac832bcbc2c5491b", + strip_prefix = "magic_get-1.0.0", + url = "https://github.com/apolukhin/magic_get/archive/1.0.0.zip", +) diff --git a/source/deps/BUILD.pfr b/source/deps/BUILD.pfr new file mode 100644 index 00000000..b083e451 --- /dev/null +++ b/source/deps/BUILD.pfr @@ -0,0 +1,28 @@ +# Copyright (c) 2020 UATC, LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +package( + default_visibility = ["//visibility:public"], +) + +cc_library( + name = "pfr", + hdrs = glob([ + "include/boost/**", + ]), + includes = [ + "include" + ], + visibility = ["//visibility:public"], +) diff --git a/source/neuropod/backends/tensorflow/tf_tensor.hh b/source/neuropod/backends/tensorflow/tf_tensor.hh index 852c847d..093e8df7 100644 --- a/source/neuropod/backends/tensorflow/tf_tensor.hh +++ b/source/neuropod/backends/tensorflow/tf_tensor.hh @@ -20,6 +20,7 @@ limitations under the License. #include "neuropod/internal/error_utils.hh" #include "neuropod/internal/logging.hh" #include "neuropod/internal/neuropod_tensor.hh" +#include "tensorflow/core/common_runtime/dma_helper.h" #include "tensorflow/core/framework/tensor.h" #include "tensorflow/core/public/version.h" @@ -104,14 +105,26 @@ protected: void *get_untyped_data_ptr() { // TODO(vip): make sure this tensor is contiguous - return const_cast(tensor_.tensor_data().data()); + auto buf = tensorflow::DMAHelper::buffer(&tensor_); + if (buf != nullptr) + { + return buf->data(); + } + + return nullptr; } // Get a pointer to the underlying data const void *get_untyped_data_ptr() const { // TODO(vip): make sure this tensor is contiguous - return tensor_.tensor_data().data(); + auto buf = tensorflow::DMAHelper::buffer(&tensor_); + if (buf != nullptr) + { + return buf->data(); + } + + return nullptr; } }; diff --git a/source/neuropod/multiprocess/BUILD b/source/neuropod/multiprocess/BUILD index 7cac5eb7..73bd382e 100644 --- a/source/neuropod/multiprocess/BUILD +++ b/source/neuropod/multiprocess/BUILD @@ -34,7 +34,6 @@ cc_library( srcs = [ "control_messages.cc", "ipc_control_channel.cc", - "ope_load_config.cc", ], hdrs = [ "control_messages.hh", diff --git a/source/neuropod/multiprocess/ope_load_config.cc b/source/neuropod/multiprocess/ope_load_config.cc deleted file mode 100644 index 0c88078e..00000000 --- a/source/neuropod/multiprocess/ope_load_config.cc +++ /dev/null @@ -1,52 +0,0 @@ -/* Copyright (c) 2020 UATC, LLC - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -#include "neuropod/multiprocess/ope_load_config.hh" - -namespace neuropod -{ - -// Specialization for BackendLoadSpec -template <> -inline void ipc_serialize(std::ostream &out, const BackendLoadSpec &data) -{ - ipc_serialize(out, data.type); - ipc_serialize(out, data.version); - ipc_serialize(out, data.path); -} - -template <> -inline void ipc_deserialize(std::istream &in, BackendLoadSpec &data) -{ - ipc_deserialize(in, data.type); - ipc_deserialize(in, data.version); - ipc_deserialize(in, data.path); -} - -template <> -void ipc_serialize(std::ostream &out, const ope_load_config &data) -{ - ipc_serialize(out, data.neuropod_path); - ipc_serialize(out, data.default_backend_overrides); -} - -template <> -void ipc_deserialize(std::istream &in, ope_load_config &data) -{ - ipc_deserialize(in, data.neuropod_path); - ipc_deserialize(in, data.default_backend_overrides); -} - -} // namespace neuropod diff --git a/source/neuropod/multiprocess/ope_load_config.hh b/source/neuropod/multiprocess/ope_load_config.hh index 71af4edb..c402d726 100644 --- a/source/neuropod/multiprocess/ope_load_config.hh +++ b/source/neuropod/multiprocess/ope_load_config.hh @@ -31,11 +31,4 @@ struct ope_load_config std::vector default_backend_overrides; }; -// Serialization specializations for ope_load_config -template <> -void ipc_serialize(std::ostream &out, const ope_load_config &data); - -template <> -void ipc_deserialize(std::istream &in, ope_load_config &data); - } // namespace neuropod diff --git a/source/neuropod/multiprocess/serialization/BUILD b/source/neuropod/multiprocess/serialization/BUILD index c3a40756..dbc78447 100644 --- a/source/neuropod/multiprocess/serialization/BUILD +++ b/source/neuropod/multiprocess/serialization/BUILD @@ -23,5 +23,6 @@ cc_library( deps = [ "//neuropod/backends:neuropod_backend", "@boost_repo//:boost", + "@pfr_repo//:pfr", ], ) diff --git a/source/neuropod/multiprocess/serialization/ipc_serialization.hh b/source/neuropod/multiprocess/serialization/ipc_serialization.hh index 96ddd981..0b9c1202 100644 --- a/source/neuropod/multiprocess/serialization/ipc_serialization.hh +++ b/source/neuropod/multiprocess/serialization/ipc_serialization.hh @@ -18,6 +18,8 @@ limitations under the License. #include "neuropod/internal/error_utils.hh" #include "neuropod/internal/memory_utils.hh" +#include + #include #include #include @@ -62,18 +64,42 @@ inline void checked_read(std::istream &stream, Params &&... params) template inline void ipc_serialize(std::ostream &out, const T &item) { - static_assert(std::is_integral::value, "The ipc_serialize function must be specialized for the requested type"); + constexpr bool is_integral = std::is_integral::value; + constexpr bool is_aggregate = std::is_aggregate::value; + + static_assert(is_integral || is_aggregate, "The ipc_serialize function must be specialized for the requested type"); - detail::checked_write(out, reinterpret_cast(&item), sizeof(item)); + if constexpr (is_integral) + { + // Primitive types + detail::checked_write(out, reinterpret_cast(&item), sizeof(item)); + } + else if (is_aggregate) + { + // Structs + boost::pfr::for_each_field(item, [&](auto &field) { ipc_serialize(out, field); }); + } } template inline void ipc_deserialize(std::istream &in, T &item) { - static_assert(std::is_integral::value, + constexpr bool is_integral = std::is_integral::value; + constexpr bool is_aggregate = std::is_aggregate::value; + + static_assert(is_integral || is_aggregate, "The ipc_deserialize function must be specialized for the requested type"); - detail::checked_read(in, reinterpret_cast(&item), sizeof(item)); + if constexpr (is_integral) + { + // Primitive types + detail::checked_read(in, reinterpret_cast(&item), sizeof(item)); + } + else if (is_aggregate) + { + // Structs + boost::pfr::for_each_field(item, [&](auto &field) { ipc_deserialize(in, field); }); + } } // Specialization for bool