-
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?
Changes from all commits
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 |
---|---|---|
|
@@ -22,10 +22,20 @@ namespace py = pybind11; | |
|
||
PYBIND11_MODULE(everestpy, m) { | ||
|
||
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); | ||
|
||
Comment on lines
+25
to
+33
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 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 commentThe 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 |
||
// FIXME (aw): add m.doc? | ||
py::class_<RuntimeSession>(m, "RuntimeSession") | ||
.def(py::init<>()) | ||
.def(py::init<const std::string&, const std::string&>()); | ||
.def(py::init<const std::string&, const std::string&>()) | ||
.def(py::init<const Everest::MQTTSettings&, const std::string&>()); | ||
|
||
py::class_<ModuleInfo::Paths>(m, "ModuleInfoPaths") | ||
.def_readonly("etc", &ModuleInfo::Paths::etc) | ||
|
@@ -130,7 +140,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 commentThe 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 |
||
.def("call_command", &Module::call_command) | ||
.def("publish_variable", &Module::publish_variable) | ||
.def("implement_command", &Module::implement_command) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,6 +53,17 @@ static Everest::MQTTSettings get_mqtt_settings_from_env() { | |
} | ||
} | ||
|
||
RuntimeSession::RuntimeSession(const Everest::MQTTSettings& mqtt_settings, const std::string& logging_config) { | ||
this->mqtt_settings = mqtt_settings; | ||
if (logging_config.empty()) { | ||
this->logging_config_file = Everest::assert_dir(Everest::defaults::PREFIX, "Default prefix") / | ||
std::filesystem::path(Everest::defaults::SYSCONF_DIR) / | ||
Everest::defaults::NAMESPACE / Everest::defaults::LOGGING_CONFIG_NAME; | ||
} else { | ||
this->logging_config_file = Everest::assert_file(logging_config, "Default logging config"); | ||
} | ||
} | ||
|
||
/// This is just kept for compatibility | ||
RuntimeSession::RuntimeSession(const std::string& prefix, const std::string& config_file) { | ||
EVLOG_warning | ||
|
@@ -63,27 +74,23 @@ 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 commentThe 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? |
||
this->logging_config_file = ms.runtime_settings->logging_config_file; | ||
|
||
this->mqtt_settings = ms.mqtt_settings; | ||
} | ||
|
||
RuntimeSession::RuntimeSession() { | ||
const auto module_id = get_variable_from_env("EV_MODULE"); | ||
|
||
namespace fs = std::filesystem; | ||
const fs::path logging_config_file = | ||
this->logging_config_file = | ||
Everest::assert_file(get_variable_from_env("EV_LOG_CONF_FILE"), "Default logging config"); | ||
Everest::Logging::init(logging_config_file.string(), module_id); | ||
|
||
this->mqtt_settings = get_mqtt_settings_from_env(); | ||
} | ||
|
||
ModuleSetup create_setup_from_config(const std::string& module_id, Everest::Config& config) { | ||
ModuleSetup setup; | ||
|
||
const std::string& module_name = config.get_main_config().at(module_id).at("module"); | ||
const auto module_manifest = config.get_manifests().at(module_name); | ||
const std::string& module_name = config.get_module_name(module_id); | ||
const auto& module_manifest = config.get_manifests().at(module_name); | ||
|
||
// setup connections | ||
for (const auto& requirement : module_manifest.at("requires").items()) { | ||
|
@@ -107,7 +114,7 @@ ModuleSetup create_setup_from_config(const std::string& module_id, Everest::Conf | |
const auto& req_route = req_route_list[i]; | ||
const auto fulfillment = | ||
Fulfillment{req_route["module_id"], req_route["implementation_id"], {requirement_id, i}}; | ||
fulfillment_list.emplace_back(std::move(fulfillment)); | ||
fulfillment_list.emplace_back(fulfillment); | ||
} | ||
} | ||
|
||
|
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) { | ||
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. 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); | ||
} | ||
|
@@ -21,7 +21,9 @@ Module::Module(const RuntimeSession& session) : Module(get_variable_from_env("EV | |
} | ||
|
||
Module::Module(const std::string& module_id_, const RuntimeSession& session_) : | ||
module_id(module_id_), session(session_) { | ||
module_id(module_id_), session(session_), start_time(std::chrono::system_clock::now()) { | ||
|
||
Everest::Logging::init(session.get_logging_config_file().string(), module_id); | ||
|
||
this->mqtt_abstraction = std::make_shared<Everest::MQTTAbstraction>(session.get_mqtt_settings()); | ||
this->mqtt_abstraction->connect(); | ||
|
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); | ||
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 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 |
---|---|---|
|
@@ -192,9 +192,10 @@ class ModuleLoader { | |
|
||
public: | ||
explicit ModuleLoader(int argc, char* argv[], ModuleCallbacks callbacks) : | ||
ModuleLoader(argc, argv, callbacks, {"undefined project", "undefined version", "undefined git version"}){}; | ||
ModuleLoader(argc, argv, std::move(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 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 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(); | ||
}; | ||
|
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 looks like a data layout problem. Not sure, if this additional data restructuring is really necessary.