From 6e477c6d2b0e9a5be4f5bf16cd967b93b4d5680d Mon Sep 17 00:00:00 2001
From: Daniel Lyubomirov <daniel.lyubomirov@vereign.com>
Date: Tue, 26 May 2020 11:45:34 +0300
Subject: [PATCH] [OPTIMIZATION] Use pimpl in the gRPC server
---
cpp/.clang-tidy | 35 +++++++
cpp/CMakeLists.txt | 27 ++++--
cpp/src/CMakeLists.txt | 18 +++-
cpp/src/csandbox.cc | 108 +---------------------
cpp/src/vereign/core/scope_guard.hh | 7 +-
cpp/src/vereign/grpc/server.cc | 137 +++++++++++++++++-----------
cpp/src/vereign/grpc/server.hh | 32 ++-----
cpp/tests/vereign/CMakeLists.txt | 6 ++
8 files changed, 172 insertions(+), 198 deletions(-)
create mode 100644 cpp/.clang-tidy
diff --git a/cpp/.clang-tidy b/cpp/.clang-tidy
new file mode 100644
index 0000000..e57be52
--- /dev/null
+++ b/cpp/.clang-tidy
@@ -0,0 +1,35 @@
+---
+Checks: 'clang-diagnostic-*,clang-analyzer-*,modernize-*,readability-*,-readability-container-size-empty'
+WarningsAsErrors: ''
+HeaderFilterRegex: ''
+AnalyzeTemporaryDtors: false
+FormatStyle: none
+CheckOptions:
+ - key: cert-dcl16-c.NewSuffixes
+ value: 'L;LL;LU;LLU'
+ - key: cert-oop54-cpp.WarnOnlyIfThisHasSuspiciousField
+ value: '0'
+ - key: cppcoreguidelines-explicit-virtual-functions.IgnoreDestructors
+ value: '1'
+ - key: cppcoreguidelines-non-private-member-variables-in-classes.IgnoreClassesWithAllMemberVariablesBeingPublic
+ value: '1'
+ - key: google-readability-braces-around-statements.ShortStatementLines
+ value: '1'
+ - key: google-readability-function-size.StatementThreshold
+ value: '800'
+ - key: google-readability-namespace-comments.ShortNamespaceLines
+ value: '10'
+ - key: google-readability-namespace-comments.SpacesBeforeComments
+ value: '2'
+ - key: modernize-loop-convert.MaxCopySize
+ value: '16'
+ - key: modernize-loop-convert.MinConfidence
+ value: reasonable
+ - key: modernize-loop-convert.NamingStyle
+ value: CamelCase
+ - key: modernize-pass-by-value.IncludeStyle
+ value: llvm
+ - key: modernize-replace-auto-ptr.IncludeStyle
+ value: llvm
+ - key: modernize-use-nullptr.NullMacros
+ value: 'NULL'
diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt
index a07568a..8ad13ac 100644
--- a/cpp/CMakeLists.txt
+++ b/cpp/CMakeLists.txt
@@ -6,6 +6,11 @@ endif()
project (vereign)
+# Options
+option(VEREIGN_USE_LLD "Use the lld linker" OFF)
+option(VEREIGN_USE_PRECOMPILED_HEADERS "Use precompiled headers" OFF)
+option(VEREIGN_USE_TIME_TRACE OFF)
+
if (CMAKE_CXX_COMPILER_ID STREQUAL "MSVC")
if (CMAKE_CXX_COMPILER_VERSION VERSION_LESS "19.0.24215.1")
message(FATAL_ERROR "Microsoft Visual C++ version MSVC 19.0.24215.1 required")
@@ -46,6 +51,9 @@ endif()
if (CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
set(CMAKE_CXX_FLAGS_RELEASE "-g -O3 -Wall -Wextra -pedantic")
set(CMAKE_CXX_FLAGS_DEBUG "-g -O0 -Wall -Wextra -pedantic")
+ if (VEREIGN_USE_LLD)
+ set(CMAKE_EXE_LINKER_FLAGS "-fuse-ld=lld")
+ endif()
endif()
if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
@@ -141,13 +149,14 @@ message(STATUS "summary of build options:
Install prefix: ${CMAKE_INSTALL_PREFIX}
Target system: ${CMAKE_SYSTEM_NAME}
Compiler:
- Build type: ${CMAKE_BUILD_TYPE}
- C compiler: ${CMAKE_C_COMPILER}
- CFLAGS: ${CMAKE_C_FLAGS_${_build_type}} ${CMAKE_C_FLAGS}
- C++ compiler: ${CMAKE_CXX_COMPILER}
- CXXFLAGS: ${CMAKE_CXX_FLAGS_${_build_type}} ${CMAKE_CXX_FLAGS}
- WARNCFLAGS: ${WARNCFLAGS}
- CXX1XCXXFLAGS: ${CXX1XCXXFLAGS}
+ Build type: ${CMAKE_BUILD_TYPE}
+ C compiler: ${CMAKE_C_COMPILER}
+ CFLAGS: ${CMAKE_C_FLAGS_${_build_type}} ${CMAKE_C_FLAGS}
+ C++ compiler: ${CMAKE_CXX_COMPILER}
+ CXXFLAGS: ${CMAKE_CXX_FLAGS_${_build_type}} ${CMAKE_CXX_FLAGS}
+ EXE_LINKER_FLAGS: ${CMAKE_EXE_LINKER_FLAGS_${_build_type}} ${CMAKE_EXE_LINKER_FLAGS}
+ WARNCFLAGS: ${WARNCFLAGS}
+ CXX1XCXXFLAGS: ${CXX1XCXXFLAGS}
Libs:
fmt: ${fmt_FOUND} [${fmt_VERSION}] (DIR='${fmt_DIR}')
OpenSSL: ${OpenSSL_FOUND} [${OPENSSL_VERSION}] (LIBS='${OPENSSL_LIBRARIES}')
@@ -155,4 +164,8 @@ message(STATUS "summary of build options:
Boost ${Boost_FOUND} [${Boost_VERSION_STRING}] (DIR='${Boost_DIR}')
Boost libs ${Boost_LIBRARIES}
gRPC ${gRPC_FOUND} [${gRPC_VERSION}] (LIBS='${_grpc_libs}')
+ Options:
+ VEREIGN_USE_LLD ${VEREIGN_USE_LLD}
+ VEREIGN_USE_PRECOMPILED_HEADERS ${VEREIGN_USE_PRECOMPILED_HEADERS}
+ VEREIGN_USE_TIME_TRACE ${VEREIGN_USE_TIME_TRACE}
")
diff --git a/cpp/src/CMakeLists.txt b/cpp/src/CMakeLists.txt
index 1c7b906..2defa3f 100644
--- a/cpp/src/CMakeLists.txt
+++ b/cpp/src/CMakeLists.txt
@@ -35,6 +35,8 @@ target_link_libraries(
OpenSSL::Crypto
$<$<CXX_COMPILER_ID:MSVC>:CRYPT32.LIB>
)
+file(GLOB PROTO_HEADERS ${CMAKE_SOURCE_DIR}/proto/cpp/vereign/client_library/*pb.h)
+target_precompile_headers(vereignproto PUBLIC ${PROTO_HEADERS})
set(VEREIGNLIB_SRC
vereign/restapi/detail/http_reader.cc
@@ -66,12 +68,18 @@ set(csandbox_sources
add_executable(csandbox ${csandbox_sources})
target_link_libraries(csandbox
- fmt::fmt
- Boost::regex
- Threads::Threads
- OpenSSL::SSL
- $<$<CXX_COMPILER_ID:MSVC>:CRYPT32.LIB>
+ vereignlib
+ # fmt::fmt
+ # Boost::regex
+ # Threads::Threads
+ # OpenSSL::SSL
+ # $<$<CXX_COMPILER_ID:MSVC>:CRYPT32.LIB>
)
+if (VEREIGN_USE_TIME_TRACE AND CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
+ target_compile_options(csandbox
+ PRIVATE "-ftime-trace"
+ )
+endif()
# Generates the gRPC and Vereign services source code.
add_custom_target(
diff --git a/cpp/src/csandbox.cc b/cpp/src/csandbox.cc
index 01b0beb..3c3bf93 100644
--- a/cpp/src/csandbox.cc
+++ b/cpp/src/csandbox.cc
@@ -1,108 +1,2 @@
-#include <functional>
-#include <memory>
-#include <thread>
-#include <future>
-#include <iostream>
-#include <type_traits>
-#include <vector>
-#include <boost/bind.hpp>
-#include <boost/optional.hpp>
-
-namespace experiment {
-
-/**
- * Array with c++11 move semantics for research purposes.
- *
- * @note Do not use this in real code.
- */
-class Array {
- public:
- Array() : arr_{nullptr}, size_{0} {
- std::cout << "default construct " << to_s() << "\n";
- }
-
- Array(std::initializer_list<int> l)
- : arr_(new int[l.size()]{})
- , size_(l.size()) {
-
- std::copy(l.begin(), l.end(), arr_);
- std::cout << "create list " << to_s() << "\n";
- }
-
- Array(const std::vector<int>& src) {
- arr_ = new int[src.size()]{};
- size_ = src.size();
- std::copy(src.begin(), src.end(), arr_);
- std::cout << "create " << to_s() << "\n";
- }
-
- Array(const Array& other) : arr_(new int[other.size_]{}), size_{other.size_} {
- std::copy(other.arr_, other.arr_ + other.size_, arr_);
- std::cout << "copy construct " << to_s() << "\n";
- }
-
- Array& operator=(const Array& other) {
- Array tmp{other};
- swap(*this, tmp);
- std::cout << "copy assign " << to_s() << "\n";
- return *this;
- }
-
- Array(Array&& other) noexcept : arr_{other.arr_}, size_(other.size_) {
- other.arr_ = nullptr;
- other.size_ = 0;
- std::cout << "move construct " << to_s() << "\n";
- }
-
- Array& operator=(Array&& other) noexcept {
- swap(*this, other);
- std::cout << "move assign " << to_s() << "\n";
- return *this;
- }
-
- ~Array() {
- std::cout << "destroy " << to_s() << "\n";
- if (arr_) {
- delete [] arr_;
- }
- }
-
- void reset() {
- if (arr_) {
- delete [] arr_;
- arr_ = nullptr;
- }
- size_ = 0;
- }
-
- std::string to_s() const {
- std::string ret = std::to_string(intptr_t(this)) + " ";
- if (!arr_) {
- return ret;
- }
-
- for (int i = 0; i < size_; i++) {
- ret += std::to_string(arr_[i]) + " ";
- }
-
- return ret;
- }
-
- friend void swap(Array& lhs, Array& rhs) noexcept;
-
- private:
- int* arr_;
- int size_;
-};
-
-void swap(Array& lhs, Array& rhs) noexcept {
- using std::swap;
- swap(lhs.arr_, rhs.arr_);
- swap(lhs.size_, rhs.size_);
-}
-}
-
-using namespace experiment;
-
-int main(int , char* []) {
+auto main(int argc, char* argv[]) -> int {
}
diff --git a/cpp/src/vereign/core/scope_guard.hh b/cpp/src/vereign/core/scope_guard.hh
index 72e4086..7c45af0 100644
--- a/cpp/src/vereign/core/scope_guard.hh
+++ b/cpp/src/vereign/core/scope_guard.hh
@@ -1,6 +1,9 @@
#ifndef __VEREIGN_CORE_SCOPE_GUARD_HH
#define __VEREIGN_CORE_SCOPE_GUARD_HH
+#include <type_traits>
+#include <utility>
+
namespace vereign {
namespace core {
@@ -23,11 +26,11 @@ private:
};
template<typename Func>
-ScopeGuard<Func> MakeScopeGuard(Func&& func) {
+auto MakeScopeGuard(Func&& func) -> ScopeGuard<Func> {
return ScopeGuard<Func>{std::move(func)};
}
}
}
-#endif
\ No newline at end of file
+#endif
diff --git a/cpp/src/vereign/grpc/server.cc b/cpp/src/vereign/grpc/server.cc
index 902cabf..c89a274 100644
--- a/cpp/src/vereign/grpc/server.cc
+++ b/cpp/src/vereign/grpc/server.cc
@@ -1,18 +1,18 @@
-#include "vereign/grpc/gen/conversation_api.hh"
-#include "vereign/service/gen/conversation_service.hh"
#include <vereign/grpc/server.hh>
-#include <vereign/grpc/gen/passport_api.hh>
-#include <vereign/service/gen/passport_service.hh>
#include <vereign/restapi/client.hh>
#include <vereign/restapi/client_session.hh>
-
#include <vereign/service/gen/gen.hh>
-#include <vereign/service/passport_service.hh>
-
#include <vereign/grpc/gen/gen.hh>
+#include <vereign/grpc/service_registry.hh>
+
+#include <vereign/service/passport_service.hh>
#include <vereign/grpc/passport_api.hh>
+#include <grpcpp/server.h>
+#include <boost/asio/io_context.hpp>
+#include <boost/asio/ssl/context.hpp>
+#include <boost/asio/executor_work_guard.hpp>
#include <grpcpp/server_builder.h>
#include <grpcpp/server_context.h>
#include <grpcpp/security/server_credentials.h>
@@ -22,14 +22,17 @@
namespace vereign {
namespace grpc {
-Server::Server(
- const std::string& listenAddress,
- const std::string& vereignHost,
- const std::string& vereignPort,
- // FIXME: the public key must come from a storage
- const std::string& publicKey
-)
- : work_guard_{asio::make_work_guard(ioc_)},
+namespace asio = boost::asio;
+
+class Server::Impl {
+public:
+ Impl(
+ const std::string& listenAddress,
+ const std::string& vereignHost,
+ const std::string& vereignPort,
+ // FIXME: the public key must come from a storage
+ const std::string& publicKey
+ ) : work_guard_{asio::make_work_guard(ioc_)},
ssl_context_{asio::ssl::context::tlsv12_client},
client_{std::make_unique<restapi::Client>(
ioc_, ssl_context_, vereignHost, vereignPort
@@ -37,37 +40,79 @@ Server::Server(
client_session_{std::make_unique<restapi::ClientSession>(
*client_, publicKey
)}
-{
- // FIXME: Verify the remote server's certificate
- // ssl_context.set_verify_mode(ssl::verify_peer);
+ {
+
+ // FIXME: Verify the remote server's certificate
+ // ssl_context.set_verify_mode(ssl::verify_peer);
+ ::grpc::ServerBuilder builder;
+ builder.AddListeningPort(
+ listenAddress,
+ ::grpc::InsecureServerCredentials(),
+ &selected_port_
+ );
+
+ // register manually written services
+ services_registry_.RegisterIfNotExist<PassportAPI<service::PassportService>>(*client_session_);
+
+ // register all generated services
+ grpc::gen::RegisterAll(*client_session_, services_registry_);
- ::grpc::ServerBuilder builder;
- builder.AddListeningPort(
- listenAddress,
- ::grpc::InsecureServerCredentials(),
- &selected_port_
- );
+ services_registry_.RegisterIntoBuilder(builder);
- // register manually written services
- services_registry_.RegisterIfNotExist<PassportAPI<service::PassportService>>(*client_session_);
+ server_ = builder.BuildAndStart();
+ if (server_ == nullptr) {
+ throw std::runtime_error("server start failed");
+ }
- // register all generated services
- grpc::gen::RegisterAll(*client_session_, services_registry_);
+ server_thread_ = std::thread([this]() {
+ server_->Wait();
+ });
- services_registry_.RegisterIntoBuilder(builder);
+ service_thread_ = std::thread([this]() {
+ ioc_.run();
+ });
+ }
+
+ void Shutdown() {
+ client_session_->Close();
+
+ server_->Shutdown();
+ if (server_thread_.joinable()) {
+ server_thread_.join();
+ }
+
+ work_guard_.reset();
+ if (service_thread_.joinable()) {
+ service_thread_.join();
+ }
+ }
- server_ = builder.BuildAndStart();
- if (server_ == nullptr) {
- throw std::runtime_error("server start failed");
+ auto SelectedPort() const -> int {
+ return selected_port_;
}
- server_thread_ = std::thread([this]() {
- server_->Wait();
- });
+private:
+ int selected_port_;
+ asio::io_context ioc_;
+ asio::executor_work_guard<asio::io_context::executor_type> work_guard_;
+ asio::ssl::context ssl_context_;
+ std::unique_ptr<vereign::restapi::Client> client_;
+ std::unique_ptr<vereign::restapi::ClientSession> client_session_;
+ ServiceRegistry services_registry_;
+ std::unique_ptr<::grpc::Server> server_;
+ std::thread server_thread_;
+ std::thread service_thread_;
+};
- service_thread_ = std::thread([this]() {
- ioc_.run();
- });
+Server::Server(
+ const std::string& listenAddress,
+ const std::string& vereignHost,
+ const std::string& vereignPort,
+ // FIXME: the public key must come from a storage
+ const std::string& publicKey
+)
+ : impl_{std::make_unique<Impl>(listenAddress, vereignHost, vereignPort, publicKey)}
+{
}
Server::~Server() {
@@ -75,21 +120,11 @@ Server::~Server() {
}
void Server::Shutdown() {
- client_session_->Close();
-
- server_->Shutdown();
- if (server_thread_.joinable()) {
- server_thread_.join();
- }
-
- work_guard_.reset();
- if (service_thread_.joinable()) {
- service_thread_.join();
- }
+ impl_->Shutdown();
}
-int Server::SelectedPort() const {
- return selected_port_;
+auto Server::SelectedPort() const -> int {
+ return impl_->SelectedPort();
}
} // namespace grpc
diff --git a/cpp/src/vereign/grpc/server.hh b/cpp/src/vereign/grpc/server.hh
index 7ba60bb..d2c4cba 100644
--- a/cpp/src/vereign/grpc/server.hh
+++ b/cpp/src/vereign/grpc/server.hh
@@ -1,24 +1,12 @@
#ifndef __VEREIGN_GRPC_SERVER_API_HH
#define __VEREIGN_GRPC_SERVER_API_HH
-#include <thread>
-#include <grpcpp/server.h>
-#include <boost/asio/io_context.hpp>
-#include <boost/asio/ssl/context.hpp>
-#include <boost/asio/executor_work_guard.hpp>
-#include <vereign/grpc/service_registry.hh>
+#include <string>
+#include <memory>
namespace vereign {
-
-namespace restapi {
-class Client;
-class ClientSession;
-}
-
namespace grpc {
-namespace asio = boost::asio;
-
/**
* Server is a grpc server that provides the Vereign services.
*
@@ -51,7 +39,7 @@ public:
// Disable copying
Server(const Server&) = delete;
- Server& operator=(const Server&) = delete;
+ auto operator=(const Server&) -> Server& = delete;
/**
* Shutdown the server.
@@ -68,19 +56,11 @@ public:
* Then this method will return the port that the OS assigned to the gRPC
* socket.
*/
- int SelectedPort() const;
+ auto SelectedPort() const -> int;
private:
- int selected_port_;
- asio::io_context ioc_;
- asio::executor_work_guard<asio::io_context::executor_type> work_guard_;
- asio::ssl::context ssl_context_;
- std::unique_ptr<vereign::restapi::Client> client_;
- std::unique_ptr<vereign::restapi::ClientSession> client_session_;
- ServiceRegistry services_registry_;
- std::unique_ptr<::grpc::Server> server_;
- std::thread server_thread_;
- std::thread service_thread_;
+ class Impl;
+ std::unique_ptr<Impl> impl_;
};
} // namespace grpc
diff --git a/cpp/tests/vereign/CMakeLists.txt b/cpp/tests/vereign/CMakeLists.txt
index d170416..16cfb8c 100644
--- a/cpp/tests/vereign/CMakeLists.txt
+++ b/cpp/tests/vereign/CMakeLists.txt
@@ -23,6 +23,12 @@ target_link_libraries(tests
Threads::Threads
)
+if (VEREIGN_USE_TIME_TRACE AND CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
+ target_compile_options(tests
+ PRIVATE "-ftime-trace"
+ )
+endif()
+
#add_custom_command(
# TARGET protobuf_tests
# COMMENT "Run protobuf tests"
--
GitLab