-
Notifications
You must be signed in to change notification settings - Fork 22
Various performance improvements #224
Changes from 1 commit
0962fe3
3f76a88
2b17dfb
f5d4333
9d4da56
9d5496c
a93bdfa
c74055b
b8385de
0bc601b
bb1c2de
0acb24f
9ddb9ce
d40f7c3
dbd4dc2
1077b00
250ee4a
5551fed
c2f2f60
b96933b
e6bf11c
5d63f80
04ec269
4b6d16e
ed5152e
30cf7e2
9a90570
c4737e3
61603c9
6c19fb4
6405e8e
7c539ac
1e4f79e
64e9ae2
9b68f6d
abad2b8
3814cdd
ad1ecda
96835c0
b42680a
560c5ab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| return std::make_unique<Everest::Everest>(module_id, config, rs.validate_schema, mqtt_abstraction, | ||
| rs.telemetry_prefix, rs.telemetry_enabled); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,7 +49,7 @@ struct ErrorFactory; | |
| class Everest { | ||
| public: | ||
| Everest(std::string module_id, const Config& config, bool validate_data_with_schema, | ||
| std::shared_ptr<MQTTAbstraction> mqtt_abstraction, const std::string& telemetry_prefix, | ||
| const std::shared_ptr<MQTTAbstraction>& mqtt_abstraction, const std::string& telemetry_prefix, | ||
| bool telemetry_enabled); | ||
|
|
||
| // forbid copy assignment and copy construction | ||
|
|
@@ -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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why const ref'ing only
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in #227 |
||
| void provide_cmd(const cmd& cmd); | ||
|
|
||
| /// | ||
|
|
@@ -217,7 +217,7 @@ class Everest { | |
| bool telemetry_enabled; | ||
| std::optional<ModuleTierMappings> module_tier_mappings; | ||
|
|
||
| void handle_ready(nlohmann::json data); | ||
| void handle_ready(const nlohmann::json& data); | ||
|
|
||
| void heartbeat(); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in #227 |
||
|
|
||
| int initialize(); | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
|
||
| /// \brief Removes a specific \p handler | ||
| void remove_handler(std::shared_ptr<TypedHandler> handler); | ||
| void remove_handler(const std::shared_ptr<TypedHandler>& handler); | ||
|
|
||
| /// \brief \returns the number of registered handlers | ||
| std::size_t count_handlers(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -78,8 +78,8 @@ static ParsedConfigMap parse_config_map(const json& config_map_schema, const jso | |
|
|
||
| // validate each config entry | ||
| for (const auto& config_entry_el : config_map_schema.items()) { | ||
| const std::string config_entry_name = config_entry_el.key(); | ||
| const json config_entry = config_entry_el.value(); | ||
| const std::string& config_entry_name = config_entry_el.key(); | ||
| const json& config_entry = config_entry_el.value(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| // only convenience exception, would be catched by schema validation below if not thrown here | ||
| if (!config_entry.contains("default") and !config_map.contains(config_entry_name)) { | ||
|
|
@@ -125,7 +125,7 @@ static auto get_provides_for_probe_module(const std::string& probe_module_id, co | |
| const auto& connections = module_config.value("connections", json::object()); | ||
|
|
||
| for (const auto& connection : connections.items()) { | ||
| const std::string req_id = connection.key(); | ||
| const std::string& req_id = connection.key(); | ||
| const std::string module_name = module_config.at("module"); | ||
| const auto& module_manifest = manifests.at(module_name); | ||
|
|
||
|
|
@@ -161,7 +161,7 @@ static auto get_provides_for_probe_module(const std::string& probe_module_id, co | |
|
|
||
| static auto get_requirements_for_probe_module(const std::string& probe_module_id, const json& config, | ||
| const json& manifests) { | ||
| const auto probe_module_config = config.at(probe_module_id); | ||
| const auto& probe_module_config = config.at(probe_module_id); | ||
|
|
||
| auto requirements = json::object(); | ||
|
|
||
|
|
@@ -579,7 +579,7 @@ void ManagerConfig::load_and_validate_manifest(const std::string& module_id, con | |
|
|
||
| // validate config for !module | ||
| { | ||
| const json config_map = module_config.at("config_module"); | ||
| const json& config_map = module_config.at("config_module"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| const json config_map_schema = this->manifests[module_config.at("module").get<std::string>()]["config"]; | ||
|
|
||
| try { | ||
|
|
@@ -1167,7 +1167,7 @@ ModuleConfigs Config::get_module_configs(const std::string& module_id) const { | |
| for (const auto& entry : conf_map.value().items()) { | ||
| const json entry_type = config_schema.at(entry.key()).at("type"); | ||
| ConfigEntry value; | ||
| const json data = entry.value(); | ||
| const json& data = entry.value(); | ||
|
||
|
|
||
| if (data.is_string()) { | ||
| value = data.get<std::string>(); | ||
|
|
@@ -1239,7 +1239,7 @@ void Config::ref_loader(const json_uri& uri, json& schema) { | |
| schema = nlohmann::json_schema::draft7_schema_builtin; | ||
| return; | ||
| } else { | ||
| const auto path = uri.path(); | ||
| const auto& path = uri.path(); | ||
| if (this->types.contains(path)) { | ||
| schema = this->types[path]; | ||
| EVLOG_verbose << fmt::format("ref path \"{}\" schema has been found.", path); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,7 +39,7 @@ const auto remote_cmd_res_timeout_seconds = 300; | |
| const std::array<std::string, 3> TELEMETRY_RESERVED_KEYS = {{"connector_id"}}; | ||
|
|
||
| Everest::Everest(std::string module_id_, const Config& config_, bool validate_data_with_schema, | ||
| std::shared_ptr<MQTTAbstraction> mqtt_abstraction, const std::string& telemetry_prefix, | ||
| const std::shared_ptr<MQTTAbstraction>& mqtt_abstraction, const std::string& telemetry_prefix, | ||
| bool telemetry_enabled) : | ||
| mqtt_abstraction(mqtt_abstraction), | ||
| config(config_), | ||
|
|
@@ -71,7 +71,8 @@ Everest::Everest(std::string module_id_, const Config& config_, bool validate_da | |
| // setup error_manager_req_global if enabled + error_database + error_state_monitor | ||
| if (this->module_manifest.contains("enable_global_errors") && | ||
| this->module_manifest.at("enable_global_errors").get<bool>()) { | ||
| std::shared_ptr<error::ErrorDatabaseMap> global_error_database = std::make_shared<error::ErrorDatabaseMap>(); | ||
| const std::shared_ptr<error::ErrorDatabaseMap>& global_error_database = | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| std::make_shared<error::ErrorDatabaseMap>(); | ||
| const error::ErrorManagerReqGlobal::SubscribeGlobalAllErrorsFunc subscribe_global_all_errors_func = | ||
| [this](const error::ErrorCallback& callback, const error::ErrorCallback& clear_callback) { | ||
| this->subscribe_global_all_errors(callback, clear_callback); | ||
|
|
@@ -90,7 +91,7 @@ Everest::Everest(std::string module_id_, const Config& config_, bool validate_da | |
| // setup error_managers, error_state_monitors, error_factories and error_databases for all implementations | ||
| for (const std::string& impl : Config::keys(this->module_manifest.at("provides"))) { | ||
| // setup shared database | ||
| std::shared_ptr<error::ErrorDatabaseMap> error_database = std::make_shared<error::ErrorDatabaseMap>(); | ||
| const std::shared_ptr<error::ErrorDatabaseMap> error_database = std::make_shared<error::ErrorDatabaseMap>(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| // setup error manager | ||
| const std::string interface_name = this->module_manifest.at("provides").at(impl).at("interface"); | ||
|
|
@@ -188,7 +189,7 @@ Everest::Everest(std::string module_id_, const Config& config_, bool validate_da | |
| } | ||
|
|
||
| // register handler for global ready signal | ||
| const auto handle_ready_wrapper = [this](const std::string&, json data) { this->handle_ready(data); }; | ||
| const auto handle_ready_wrapper = [this](const std::string&, const json& data) { this->handle_ready(data); }; | ||
| const auto everest_ready = | ||
| std::make_shared<TypedHandler>(HandlerType::ExternalMQTT, std::make_shared<Handler>(handle_ready_wrapper)); | ||
| this->mqtt_abstraction->register_handler(fmt::format("{}ready", mqtt_everest_prefix), everest_ready, QOS::QOS2); | ||
|
|
@@ -265,7 +266,7 @@ void Everest::check_code() { | |
| this->config.get_manifests()[this->config.get_main_config()[this->module_id]["module"].get<std::string>()]; | ||
| for (const auto& element : module_manifest.at("provides").items()) { | ||
| const auto& impl_id = element.key(); | ||
| const auto impl_manifest = element.value(); | ||
| const auto& impl_manifest = element.value(); | ||
| const auto interface_definition = this->config.get_interface_definition(impl_manifest.at("interface")); | ||
|
|
||
| std::set<std::string> cmds_not_registered; | ||
|
|
@@ -666,13 +667,13 @@ void Everest::subscribe_global_all_errors(const error::ErrorCallback& callback, | |
| for (const auto& [module_id, module_name] : this->config.get_module_names()) { | ||
| const json provides = this->config.get_manifests().at(module_name).at("provides"); | ||
| for (const auto& impl : provides.items()) { | ||
| const std::string impl_id = impl.key(); | ||
| const std::string& impl_id = impl.key(); | ||
| const std::string interface = impl.value().at("interface"); | ||
| const json errors = this->config.get_interface_definition(interface).at("errors"); | ||
| for (const auto& error_namespace_it : errors.items()) { | ||
| const std::string error_type_namespace = error_namespace_it.key(); | ||
| const std::string& error_type_namespace = error_namespace_it.key(); | ||
| for (const auto& error_name_it : error_namespace_it.value().items()) { | ||
| const std::string error_type_name = error_name_it.key(); | ||
| const std::string& error_type_name = error_name_it.key(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are 4 nested for loops here, I would appreciate, that if places like these get touched, that they get refactored or a FIXME gets added.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This gets refactored in #225 |
||
| const std::string raise_topic = | ||
| fmt::format("{}/error/{}/{}", this->config.mqtt_prefix(module_id, impl_id), | ||
| error_type_namespace, error_type_name); | ||
|
|
@@ -784,7 +785,7 @@ void Everest::signal_ready() { | |
| /// \brief Ready handler for global readyness (e.g. all modules are ready now). | ||
| /// This will called when receiving the global ready signal from manager. | ||
| /// | ||
| void Everest::handle_ready(json data) { | ||
| void Everest::handle_ready(const json& data) { | ||
| BOOST_LOG_FUNCTION(); | ||
|
|
||
| EVLOG_debug << fmt::format("handle_ready: {}", data.dump()); | ||
|
|
@@ -818,7 +819,7 @@ void Everest::handle_ready(json data) { | |
| // this->heartbeat_thread = std::thread(&Everest::heartbeat, this); | ||
| } | ||
|
|
||
| void Everest::provide_cmd(const std::string impl_id, const std::string cmd_name, const JsonCommand handler) { | ||
| void Everest::provide_cmd(const std::string& impl_id, const std::string cmd_name, const JsonCommand& handler) { | ||
| BOOST_LOG_FUNCTION(); | ||
|
|
||
| // extract manifest definition of this command | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,7 +70,7 @@ MessageHandler::MessageHandler() : running(true) { | |
| std::vector<std::shared_ptr<TypedHandler>> local_handlers; | ||
| { | ||
| const std::lock_guard<std::mutex> handlers_lock(handler_list_mutex); | ||
| for (auto handler : this->handlers) { | ||
| for (const auto& handler : this->handlers) { | ||
| local_handlers.push_back(handler); | ||
| } | ||
|
Comment on lines
+73
to
75
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't be |
||
| } | ||
|
|
@@ -129,14 +129,14 @@ void MessageHandler::stop() { | |
| this->cv.notify_all(); | ||
| } | ||
|
|
||
| void MessageHandler::add_handler(std::shared_ptr<TypedHandler> handler) { | ||
| void MessageHandler::add_handler(const std::shared_ptr<TypedHandler>& handler) { | ||
| { | ||
| const std::lock_guard<std::mutex> lock(this->handler_list_mutex); | ||
| this->handlers.insert(handler); | ||
| } | ||
| } | ||
|
|
||
| void MessageHandler::remove_handler(std::shared_ptr<TypedHandler> handler) { | ||
| void MessageHandler::remove_handler(const std::shared_ptr<TypedHandler>& handler) { | ||
| { | ||
| const std::lock_guard<std::mutex> lock(this->handler_list_mutex); | ||
| auto it = std::find(this->handlers.begin(), this->handlers.end(), 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.
This might be a matter of taste. I'm just wondering if now any appearance of any
std::functionwill be passed by reference instead of value?