Add a few simple performance enhancements (#72)

* Default initialize validate_schema with false * Only validate var subscription if validate_data_with_schema is set * Add some caching of module info * Make some data accesses a bit more efficient * Pass topic to check_topic_matches as const ref Signed-off-by: Kai-Uwe Hermann <[email protected]> --------- Signed-off-by: Kai-Uwe Hermann <[email protected]>
EVerest · Feb 23, 2023 · 0576ad1 · 0576ad1
1 parent 7afb87f
commit 0576ad1
Show file tree

Hide file tree

Showing 7 changed files with 95 additions and 39 deletions.
diff --git a/include/utils/config.hpp b/include/utils/config.hpp
@@ -11,6 +11,7 @@
 
 #include <nlohmann/json-schema.hpp>
 
+#include <utils/config_cache.hpp>
 #include <utils/types.hpp>
 
 namespace Everest {
@@ -85,7 +86,14 @@ class Config {
     json extract_implementation_info(const std::string& module_id, const std::string& impl_id);
     void resolve_all_requirements();
 
+    // experimental caches
+    std::unordered_map<std::string, std::string> module_names;
+    std::unordered_map<std::string, ConfigCache> module_config_cache;
+
 public:
+    std::string get_module_name(const std::string& module_id);
+    bool module_provides(const std::string& module_name, const std::string& impl_id);
+    json get_module_cmds(const std::string& module_name, const std::string& impl_id);
     ///
     /// \brief creates a new Config object, looking for the config.json and schemes folder relative to the provided \p
     /// main_dir

diff --git a/include/utils/config_cache.hpp b/include/utils/config_cache.hpp
@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: Apache-2.0
+// Copyright 2020 - 2023 Pionix GmbH and Contributors to EVerest
+#ifndef UTILS_CONFIG_CACHE_HPP
+#define UTILS_CONFIG_CACHE_HPP
+
+#include <set>
+#include <string>
+#include <unordered_map>
+
+#include <utils/types.hpp>
+
+namespace Everest {
+using json = nlohmann::json;
+
+struct ConfigCache {
+    std::set<std::string> provides_impl;
+    std::unordered_map<std::string, json> cmds;
+};
+
+} // namespace Everest
+
+#endif // UTILS_CONFIG_CACHE_HPP
diff --git a/include/utils/mqtt_abstraction_impl.hpp b/include/utils/mqtt_abstraction_impl.hpp
@@ -122,7 +122,7 @@ class MQTTAbstractionImpl {
     /// wildcards
     ///
     /// \returns true if the topic matches, false otherwise
-    static bool check_topic_matches(std::string full_topic, std::string wildcard_topic);
+    static bool check_topic_matches(const std::string& full_topic, const std::string& wildcard_topic);
 
     ///
     /// \brief callback that is called from the mqtt implementation whenever a message is received

diff --git a/lib/config.cpp b/lib/config.cpp
@@ -187,6 +187,8 @@ Config::Config(std::string schemas_dir, std::string config_file, std::string mod
         auto& module_config = element.value();
         std::string module_name = module_config["module"];
 
+        this->module_config_cache[module_id] = ConfigCache();
+        this->module_names[module_id] = module_name;
         EVLOG_debug << fmt::format("Found module {}, loading and verifying manifest...",
                                    printable_identifier(module_id));
 
@@ -232,12 +234,15 @@ Config::Config(std::string schemas_dir, std::string config_file, std::string mod
         std::set<std::string> provided_impls = Config::keys(this->manifests[module_name]["provides"]);
 
         this->interfaces[module_name] = json({});
+        this->module_config_cache[module_name].provides_impl = provided_impls;
 
         for (const auto& impl_id : provided_impls) {
             EVLOG_debug << fmt::format("Loading interface for implementation: {}", impl_id);
             auto intf_name = this->manifests[module_name]["provides"][impl_id]["interface"].get<std::string>();
             auto seen_interfaces = std::set<std::string>();
             this->interfaces[module_name][impl_id] = resolve_interface(intf_name);
+            this->module_config_cache[module_name].cmds[impl_id] =
+                this->interfaces.at(module_name).at(impl_id).at("cmds");
         }
 
         // check if config only contains impl_ids listed in manifest file
@@ -325,6 +330,19 @@ Config::Config(std::string schemas_dir, std::string config_file, std::string mod
     resolve_all_requirements();
 }
 
+std::string Config::get_module_name(const std::string& module_id) {
+    return this->module_names.at(module_id);
+}
+
+bool Config::module_provides(const std::string& module_name, const std::string& impl_id) {
+    auto& provides = this->module_config_cache.at(module_name).provides_impl;
+    return (provides.find(impl_id) != provides.end());
+}
+
+json Config::get_module_cmds(const std::string& module_name, const std::string& impl_id) {
+    return this->module_config_cache.at(module_name).cmds.at(impl_id);
+}
+
 json Config::resolve_interface(const std::string& intf_name) {
     // load and validate interface.json and mark interface as seen
     auto intf_definition = load_interface_file(intf_name);
@@ -378,14 +396,15 @@ json Config::resolve_requirement(const std::string& module_id, const std::string
     //             isn't even listed in the module manifest
     // FIXME (aw): the following if doesn't check for the requirement id
     //             at all
-    if (!this->main.contains(module_id)) {
+    auto module_name_it = this->module_names.find(module_id);
+    if (module_name_it == this->module_names.end()) {
         EVLOG_AND_THROW(EverestApiError(fmt::format("Requested requirement id '{}' of module {} not found in config!",
                                                     requirement_id, printable_identifier(module_id))));
     }
 
     // check for connections for this requirement
-    json module_config = this->main[module_id];
-    std::string module_name = module_config["module"].get<std::string>();
+    auto& module_config = this->main[module_id];
+    std::string module_name = module_name_it->second;
     auto& requirement = this->manifests[module_name]["requires"][requirement_id];
     if (!module_config["connections"].contains(requirement_id)) {
         return json::array(); // return an empty array if our config does not contain any connections for this
@@ -687,7 +706,7 @@ json Config::extract_implementation_info(const std::string& module_id, const std
 
     json info;
     info["module_id"] = module_id;
-    info["module_name"] = this->main[module_id]["module"];
+    info["module_name"] = get_module_name(module_id);
     info["impl_id"] = impl_id;
     info["impl_intf"] = "";
     if (!impl_id.empty()) {

diff --git a/lib/everest.cpp b/lib/everest.cpp
@@ -220,20 +220,21 @@ json Everest::call_cmd(const Requirement& req, const std::string& cmd_name, json
     std::future<json> res_future = res_promise.get_future();
 
     Handler res_handler = [this, &res_promise, call_id, connection, cmd_name, return_type](json data) {
-        if (data["id"] != call_id) {
-            EVLOG_debug << fmt::format("RES: data_id != call_id ({} != {})", data["id"], call_id);
+        auto& data_id = data.at("id");
+        if (data_id != call_id) {
+            EVLOG_debug << fmt::format("RES: data_id != call_id ({} != {})", data_id, call_id);
             return;
         }
 
         EVLOG_debug << fmt::format(
-            "Incoming res {} for {}->{}()", data["id"],
+            "Incoming res {} for {}->{}()", data_id,
             this->config.printable_identifier(connection["module_id"], connection["implementation_id"]), cmd_name);
 
         // make sure to only return the intended parts of the incoming result to not open up the api to internals
         res_promise.set_value(json::object({{"retval", data["retval"]},
                                             {"return_type", return_type},
                                             {"origin", data["origin"]},
-                                            {"id", data["id"]}}));
+                                            {"id", data_id}}));
     };
 
     const auto cmd_topic =
@@ -338,7 +339,7 @@ void Everest::subscribe_var(const Requirement& req, const std::string& var_name,
     }
 
     auto requirement_module_id = connection["module_id"].get<std::string>();
-    auto module_name = this->config.get_main_config()[requirement_module_id]["module"].get<std::string>();
+    auto module_name = this->config.get_module_name(requirement_module_id);
     auto requirement_impl_id = connection["implementation_id"].get<std::string>();
     auto requirement_impl_manifest = this->config.get_interfaces()[module_name][requirement_impl_id];
 
@@ -355,17 +356,19 @@ void Everest::subscribe_var(const Requirement& req, const std::string& var_name,
         EVLOG_debug << fmt::format(
             "Incoming {}->{}", this->config.printable_identifier(requirement_module_id, requirement_impl_id), var_name);
 
-        // check data and ignore it if not matching (publishing it should have been prohibited already)
-        try {
-            json_validator validator(
-                [this](const json_uri& uri, json& schema) { this->config.ref_loader(uri, schema); },
-                Config::format_checker);
-            validator.set_root_schema(requirement_manifest_vardef);
-            validator.validate(data);
-        } catch (const std::exception& e) {
-            EVLOG_warning << fmt::format("Ignoring incoming var '{}' because not matching manifest schema: {}",
-                                         var_name, e.what());
-            return;
+        if (this->validate_data_with_schema) {
+            // check data and ignore it if not matching (publishing it should have been prohibited already)
+            try {
+                json_validator validator(
+                    [this](const json_uri& uri, json& schema) { this->config.ref_loader(uri, schema); },
+                    Config::format_checker);
+                validator.set_root_schema(requirement_manifest_vardef);
+                validator.validate(data);
+            } catch (const std::exception& e) {
+                EVLOG_warning << fmt::format("Ignoring incoming var '{}' because not matching manifest schema: {}",
+                                            var_name, e.what());
+                return;
+            }
         }
 
         callback(data);
@@ -389,7 +392,7 @@ void Everest::subscribe_var(const Requirement& req, const std::string& var_name,
     }
 
     auto requirement_module_id = connection["module_id"].get<std::string>();
-    auto module_name = this->config.get_main_config()[requirement_module_id]["module"].get<std::string>();
+    auto module_name = this->config.get_module_name(requirement_module_id);
     auto requirement_impl_id = connection["implementation_id"].get<std::string>();
     auto requirement_impl_manifest = this->config.get_interfaces()[module_name][requirement_impl_id];
 
@@ -687,12 +690,10 @@ json Everest::get_cmd_definition(const std::string& module_id, const std::string
                                  bool is_call) {
     BOOST_LOG_FUNCTION();
 
-    std::string module_name = this->config.get_main_config()[module_id]["module"].get<std::string>();
-    auto module_manifest = this->config.get_manifests()[module_name];
-    auto module = this->config.get_interfaces()[module_name];
-    auto impl_intf = module[impl_id];
+    std::string module_name = this->config.get_module_name(module_id);
+    auto cmds = this->config.get_module_cmds(module_name, impl_id);
 
-    if (!module_manifest["provides"].contains(impl_id)) {
+    if (!this->config.module_provides(module_name, impl_id)) {
         if (!is_call) {
             EVLOG_AND_THROW(EverestApiError(fmt::format(
                 "Module {} tries to provide implementation '{}' not declared in manifest!", module_name, impl_id)));
@@ -703,7 +704,7 @@ json Everest::get_cmd_definition(const std::string& module_id, const std::string
         }
     }
 
-    if (!impl_intf["cmds"].contains(cmd_name)) {
+    if (!cmds.contains(cmd_name)) {
         if (!is_call) {
             EVLOG_AND_THROW(
                 EverestApiError(fmt::format("{} tries to provide cmd '{}' not declared in manifest!",
@@ -715,7 +716,7 @@ json Everest::get_cmd_definition(const std::string& module_id, const std::string
         }
     }
 
-    return impl_intf["cmds"][cmd_name];
+    return cmds.at(cmd_name);
 }
 
 json Everest::get_cmd_definition(const std::string& module_id, const std::string& impl_id,
@@ -751,8 +752,10 @@ bool Everest::check_arg(ArgumentType arg_types, json manifest_arg) {
     // FIXME (aw): the error messages here need to be taken into the
     //             correct context!
 
-    if (manifest_arg["type"].is_string()) {
-        if (manifest_arg["type"] == "null") {
+    auto& manifest_arg_type = manifest_arg.at("type");
+
+    if (manifest_arg_type.is_string()) {
+        if (manifest_arg_type == "null") {
             // arg_types should be empty if the type is null (void)
             if (arg_types.size()) {
                 EVLOG_error << "expeceted 'null' type, but got another type";
@@ -762,16 +765,16 @@ bool Everest::check_arg(ArgumentType arg_types, json manifest_arg) {
         }
         // direct comparison
         // FIXME (aw): arg_types[0] access should be checked, otherwise core dumps
-        if (arg_types[0] != manifest_arg["type"]) {
-            EVLOG_error << fmt::format("types do not match: {} != {}", arg_types[0], manifest_arg["type"]);
+        if (arg_types[0] != manifest_arg_type) {
+            EVLOG_error << fmt::format("types do not match: {} != {}", arg_types[0], manifest_arg_type);
             return false;
         }
         return true;
     }
 
     for (size_t i = 0; i < arg_types.size(); i++) {
-        if (arg_types[i] != manifest_arg["type"][i]) {
-            EVLOG_error << fmt::format("types do not match: {} != {}", arg_types[i], manifest_arg["type"][i]);
+        if (arg_types[i] != manifest_arg_type.at(i)) {
+            EVLOG_error << fmt::format("types do not match: {} != {}", arg_types[i], manifest_arg_type.at(i));
             return false;
         }
     }

diff --git a/lib/mqtt_abstraction_impl.cpp b/lib/mqtt_abstraction_impl.cpp
@@ -318,9 +318,12 @@ void MQTTAbstractionImpl::unregister_handler(const std::string& topic, const Tok
 
     const std::lock_guard<std::mutex> lock(handlers_mutex);
     auto number_of_handlers = 0;
-    if (this->message_handlers.count(topic) > 0 && this->message_handlers[topic].count_handlers() != 0) {
-        this->message_handlers[topic].remove_handler(token);
-        number_of_handlers = this->message_handlers[topic].count_handlers();
+    if (this->message_handlers.find(topic) != this->message_handlers.end()) {
+        auto& topic_message_handler = this->message_handlers.at(topic);
+        if (topic_message_handler.count_handlers() != 0) {
+            topic_message_handler.remove_handler(token);
+            number_of_handlers = topic_message_handler.count_handlers();
+        }
     }
 
     // unsubscribe if this was the last handler for this topic
@@ -411,7 +414,7 @@ int MQTTAbstractionImpl::open_nb_socket(const char* addr, const char* port) {
 }
 
 // NOLINTNEXTLINE(misc-no-recursion)
-bool MQTTAbstractionImpl::check_topic_matches(std::string full_topic, std::string wildcard_topic) {
+bool MQTTAbstractionImpl::check_topic_matches(const std::string& full_topic, const std::string& wildcard_topic) {
     BOOST_LOG_FUNCTION();
 
     // verbatim topic

diff --git a/lib/runtime.cpp b/lib/runtime.cpp
@@ -295,6 +295,7 @@ RuntimeSettings::RuntimeSettings(const std::string& prefix_, const std::string&
     } else {
         telemetry_enabled = defaults::TELEMETRY_ENABLED;
     }
+    validate_schema = false;
 }
 
 ModuleCallbacks::ModuleCallbacks(const std::function<void(ModuleAdapter module_adapter)>& register_module_adapter,