-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Various performance improvements #224
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
The same functionality can be achieved without first including the main config in the serialized config and then deleting it... Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Now when a schema is loaded a validator for it is instantiated as well Then validators can then be re-used instead of instantiating new ones for the same schema multiple times Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Skip storing it in a temporary variable Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
…ount of space in the string the file content gets read into Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
…imum length Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
…etain Previously this was sent to every module individually which isn't necessary Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Add type to parsed_config_map and remove config entry from manifest sent via MQTT Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
refactor interfaces, manifests and types get_async Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
…e is required Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
…logging configuration directly Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
…sion ctor that accepts config file Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
…rtup Remove them by default now but allow them to be explicitly kept for inspection Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
…ed by the MQTT abstraction Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
This drastically reduces the amount of lingering threads just hanging around and doing absolutely nothing Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
…mprovements Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are quite a lot of commits. I would appreciate, if these could be split into several PRs, focused on their real use case like refactor, bugfix, features etc...
@@ -130,7 +130,7 @@ PYBIND11_MODULE(everestpy, m) { | |||
.def(py::init<const std::string&, const RuntimeSession&>()) | |||
.def("say_hello", &Module::say_hello) | |||
.def("init_done", py::overload_cast<>(&Module::init_done)) | |||
.def("init_done", py::overload_cast<std::function<void()>>(&Module::init_done)) | |||
.def("init_done", py::overload_cast<const std::function<void()>&>(&Module::init_done)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a matter of taste. I'm just wondering if now any appearance of any std::function
will be passed by reference instead of value?
@@ -12,7 +12,7 @@ | |||
std::unique_ptr<Everest::Everest> | |||
Module::create_everest_instance(const std::string& module_id, const Everest::Config& config, | |||
const Everest::RuntimeSettings& rs, | |||
std::shared_ptr<Everest::MQTTAbstraction> mqtt_abstraction) { | |||
const std::shared_ptr<Everest::MQTTAbstraction>& mqtt_abstraction) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A constant reference to a shared pointer is almost the same as the pointer itself. Everest::MQTTAbstraction&
would be the better choice here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it would have probably been fine because Module will outlive Everest, but I've reverted the change in #227
@@ -66,7 +66,7 @@ class Everest { | |||
/// | |||
/// \brief Allows a module to indicate that it provides the given command \p cmd | |||
/// | |||
void provide_cmd(const std::string impl_id, const std::string cmd_name, const JsonCommand handler); | |||
void provide_cmd(const std::string& impl_id, const std::string cmd_name, const JsonCommand& handler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why const ref'ing only impl_id
and not cmd_name
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in #227
@@ -194,7 +194,7 @@ class ModuleLoader { | |||
explicit ModuleLoader(int argc, char* argv[], ModuleCallbacks callbacks) : | |||
ModuleLoader(argc, argv, callbacks, {"undefined project", "undefined version", "undefined git version"}){}; | |||
explicit ModuleLoader(int argc, char* argv[], ModuleCallbacks callbacks, | |||
const VersionInformation version_information); | |||
const VersionInformation& version_information); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should potentially passed by value, as this information is probably only moved to the ModuleLoader
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in #227
@@ -79,10 +79,10 @@ class MessageHandler { | |||
void stop(); | |||
|
|||
/// \brief Adds a \p handler that will receive messages from the queue. | |||
void add_handler(std::shared_ptr<TypedHandler> handler); | |||
void add_handler(const std::shared_ptr<TypedHandler>& handler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above - I think this might not be a good choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed this (by reverting the change and blocking the suggestion of adding const ref to shared_ptr args) in #227
I think this was changed by me just because I wasn't paying too close attention to the recommendation of clang-tidy here
const std::string config_topic = fmt::format("{}/config", config.mqtt_module_prefix(module_name)); | ||
const Handler module_get_config_handler = [module_name, config_topic, serialized_mod_config, | ||
&mqtt_abstraction](const std::string&, const nlohmann::json& json) { | ||
mqtt_abstraction.publish(config_topic, serialized_mod_config.dump(), QOS::QOS2); | ||
}; | ||
|
||
const std::string get_config_topic = fmt::format("{}/get_config", config.mqtt_module_prefix(module_name)); | ||
module_it->second.get_config_token = std::make_shared<TypedHandler>( | ||
HandlerType::ExternalMQTT, std::make_shared<Handler>(module_get_config_handler)); | ||
mqtt_abstraction.register_handler(get_config_topic, module_it->second.get_config_token, QOS::QOS2); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for this rearrangement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking was that since the spawned module will first call get_config
and later on ready
this should also be reflected in the code here
@@ -63,18 +63,14 @@ RuntimeSession::RuntimeSession(const std::string& prefix, const std::string& con | |||
// We extract the settings from the config file so everest-testing doesn't break | |||
const auto ms = Everest::ManagerSettings(prefix, config_file); | |||
|
|||
Everest::Logging::init(ms.runtime_settings->logging_config_file.string()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why has this been moved? Isn't logging already used before until the module is constructed?
py::class_<Everest::MQTTSettings>(m, "MQTTSettings") | ||
.def(py::init<>()) | ||
.def_readwrite("broker_socket_path", &Everest::MQTTSettings::broker_socket_path) | ||
.def_readwrite("broker_host", &Everest::MQTTSettings::broker_host) | ||
.def_readwrite("broker_port", &Everest::MQTTSettings::broker_port) | ||
.def_readwrite("everest_prefix", &Everest::MQTTSettings::everest_prefix) | ||
.def_readwrite("external_prefix", &Everest::MQTTSettings::external_prefix) | ||
.def("uses_socket", &Everest::MQTTSettings::uses_socket); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why has this been added? The more we broaden the api, the harder it gets to refactor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows everest-testing to use the MQTT based config distribution mechanism since it's (as far as I know) almost impossible to set the environment variables in the pytest environment. Usage is implemented here: EVerest/everest-utils#173
@@ -80,6 +80,10 @@ class MQTTAbstractionImpl { | |||
/// \brief unsubscribes from the given \p topic | |||
void unsubscribe(const std::string& topic); | |||
|
|||
/// | |||
/// \brief clears any previously published topics that had the retain flag set | |||
void clear_retained_topics(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any documentation on how or when the retain-topics
flag should be used?
@@ -749,6 +753,9 @@ int boot(const po::variables_map& vm) { | |||
|
|||
auto module_handles = | |||
start_modules(*config, mqtt_abstraction, ignored_modules, standalone_modules, ms, status_fifo, retain_topics); | |||
if (module_handles.empty()) { | |||
return EXIT_FAILURE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be considered a failure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's certainly debatable. The current behavior is that everest will happily load empty configs, but it doesn't signal a completed startup via the status fifo or logs. So for a user it just "hangs" without doing anything else. I thought failing early would be better in this case, but we can also leave the current behavior in place if that's more desirable
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
… is only used there Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@@ -66,7 +66,7 @@ class Everest { | |||
/// | |||
/// \brief Allows a module to indicate that it provides the given command \p cmd | |||
/// | |||
void provide_cmd(const std::string impl_id, const std::string cmd_name, const JsonCommand handler); | |||
void provide_cmd(const std::string& impl_id, const std::string cmd_name, const JsonCommand& handler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in #227
@@ -194,7 +194,7 @@ class ModuleLoader { | |||
explicit ModuleLoader(int argc, char* argv[], ModuleCallbacks callbacks) : | |||
ModuleLoader(argc, argv, callbacks, {"undefined project", "undefined version", "undefined git version"}){}; | |||
explicit ModuleLoader(int argc, char* argv[], ModuleCallbacks callbacks, | |||
const VersionInformation version_information); | |||
const VersionInformation& version_information); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in #227
@@ -20,7 +20,7 @@ | |||
|
|||
#include <utils/thread.hpp> | |||
|
|||
constexpr std::size_t MQTT_BUF_SIZE = 500 * 1024; | |||
constexpr std::size_t MQTT_BUF_SIZE = std::size_t{500} * std::size_t{1024}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::size_t{500 * 1024}
still produces a widening conversion warning since 500 and 1024 are interpreted as string. In C++23 we could postfix it with uz
but I think this way is fine for now (it only fixes a warning anyway). Addressed in #227
static std::streamsize clamp(std::size_t len) { | ||
return (len <= std::numeric_limits<std::streamsize>::max()) ? static_cast<std::streamsize>(len) | ||
: std::numeric_limits<std::streamsize>::max(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in #227
lib/yaml_loader.cpp
Outdated
std::ifstream ifs(path.string()); | ||
ifs.seekg(0, std::ios::end); | ||
std::string content; | ||
content.reserve(ifs.tellg()); | ||
ifs.seekg(0); | ||
content.assign(std::istreambuf_iterator<char>(ifs), std::istreambuf_iterator<char>()); | ||
return content; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, I did some quick comparisons between the two and didn't really see much different one way or the other so I reverted this back to the original code
src/manager.cpp
Outdated
@@ -638,7 +638,7 @@ int boot(const po::variables_map& vm) { | |||
EVLOG_info << ms.version_information; | |||
EVLOG_info << ""; | |||
|
|||
if (ms.mqtt_settings.uses_socket()) { | |||
if (not ms.mqtt_settings.uses_socket()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in #228
lib/config.cpp
Outdated
SchemaValidation Config::load_schemas(const fs::path& schemas_dir) { | ||
BOOST_LOG_FUNCTION(); | ||
schemas schemas; | ||
SchemaValidation schema_validation; | ||
|
||
EVLOG_debug << fmt::format("Loading base schema files for config and manifests... from: {}", schemas_dir.string()); | ||
schemas.config = Config::load_schema(schemas_dir / "config.yaml"); | ||
schemas.manifest = Config::load_schema(schemas_dir / "manifest.yaml"); | ||
schemas.interface = Config::load_schema(schemas_dir / "interface.yaml"); | ||
schemas.type = Config::load_schema(schemas_dir / "type.yaml"); | ||
schemas.error_declaration_list = Config::load_schema(schemas_dir / "error-declaration-list.yaml"); | ||
|
||
return schemas; | ||
auto [config_schema, config_val] = Config::load_schema(schemas_dir / "config.yaml"); | ||
schema_validation.schemas.config = config_schema; | ||
schema_validation.validators.config = std::move(config_val); | ||
auto [manifest_schema, manifest_val] = Config::load_schema(schemas_dir / "manifest.yaml"); | ||
schema_validation.schemas.manifest = manifest_schema; | ||
schema_validation.validators.manifest = std::move(manifest_val); | ||
auto [interface_schema, interface_val] = Config::load_schema(schemas_dir / "interface.yaml"); | ||
schema_validation.schemas.interface = interface_schema; | ||
schema_validation.validators.interface = std::move(interface_val); | ||
auto [type_schema, type_val] = Config::load_schema(schemas_dir / "type.yaml"); | ||
schema_validation.schemas.type = type_schema; | ||
schema_validation.validators.type = std::move(type_val); | ||
auto [error_declaration_list_schema, error_declaration_list_val] = | ||
Config::load_schema(schemas_dir / "error-declaration-list.yaml"); | ||
schema_validation.schemas.error_declaration_list = error_declaration_list_schema; | ||
schema_validation.validators.error_declaration_list = std::move(error_declaration_list_val); | ||
|
||
return schema_validation; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I've turned both load_schema
and load_schemas
into free functions
include/utils/config.hpp
Outdated
std::unique_ptr<nlohmann::json_schema::json_validator> config; | ||
std::unique_ptr<nlohmann::json_schema::json_validator> manifest; | ||
std::unique_ptr<nlohmann::json_schema::json_validator> type; | ||
std::unique_ptr<nlohmann::json_schema::json_validator> interface; | ||
std::unique_ptr<nlohmann::json_schema::json_validator> error_declaration_list; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
include/utils/mqtt_abstraction.hpp
Outdated
@@ -64,6 +64,10 @@ class MQTTAbstraction { | |||
/// \copydoc MQTTAbstractionImpl::unsubscribe(const std::string&) | |||
void unsubscribe(const std::string& topic); | |||
|
|||
/// | |||
/// \copydoc MQTTAbstractionImpl::get_async(const std::string&, QOS) | |||
AsyncReturn get_async(const std::string& topic, QOS qos); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this (and get() as well) into module_config.cpp since it is only used there
@@ -484,6 +472,7 @@ void MQTTAbstractionImpl::unregister_handler(const std::string& topic, const Tok | |||
EVLOG_verbose << fmt::format("Unsubscribing from {}", topic); | |||
this->unsubscribe(topic); | |||
} | |||
this->message_handlers.erase(topic); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened a new PR for this change which helps reduce the amount of idle threads quite significantly: #229
In general this design needs to be refactored, but that's probably best done in a future PR
…mprovements Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
contains performance enhancements in:
Additionally addresses some clang-tidy issues
Adds a new ctor for everestpy to allow MQTT based config distribution to be used in everest-testing more easily
Remove retained topics after startup and add flag that prevents this from happening for introspection
Some quick-and-dirty performance comparisons starting up the config-sil with a recent everest-core main. This mostly shows that config loading got faster (by cleaning up the usage of schema validators) and a decrease in individual module startup time. Interestingly the total startup time is pretty similar which warrants further investigation:
(v0.19.2 vs v0.19.1)
vs.
Comparing the amount of threads a module uses shows a measurable decrease (refactoring the error handling to use less topics/handlers will have an even big impact on eg. ErrorHistory #225 )
(v0.19.2 vs v0.19.1)
vs