From 076fd08ba8a42b7227e114d4bbc84627eb988b6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mattias=20Sch=C3=A4ffersmann?= Date: Thu, 17 Oct 2024 18:15:05 +0200 Subject: [PATCH] config: Add ConfObject::get() that accepts string literals Almost all ConfObject::get() calls pass string literals, which are de-duplicated when placed in the rodata section with the string literals that were used to construct the ConfObject. Pass these strings through to the getter and use them for a fast address comparison instead of doing a slower string comparison on every key in the ConfObject. Unfortunately, the overloaded get(const char *) causes a conflict with ConfArray::get(size_t) because any use like get(1) will look for an int variant of get(), which will also match the const char * variant. Add several get() overloads for various integer types to weed out the undesirable ones. The int variant is also undesirable, but unfortunately required for get(1) to work. All get-wrappers can be inlined and optimized away. Also change the ConfirmKey convenience function to const char *, so that the optimized string address comparison works with it as well. --- software/src/config.cpp | 27 +++++++++------- software/src/config.h | 20 ++++++++++-- software/src/config/conf_object.cpp | 50 ++++++++++++++++++++++------- 3 files changed, 71 insertions(+), 26 deletions(-) diff --git a/software/src/config.cpp b/software/src/config.cpp index 6e2980d0f..bd4d397c9 100644 --- a/software/src/config.cpp +++ b/software/src/config.cpp @@ -205,11 +205,6 @@ ConfigRoot *Config::Confirm() return &confirmconf; } -String Config::ConfirmKey() -{ - return Config::confirm_key; -} - Config Config::Uint8(uint8_t u) { return Config::Uint(u, std::numeric_limits::lowest(), std::numeric_limits::max()); @@ -278,28 +273,38 @@ static void abort_on_object_get_failure(const Config *conf, const char *key) esp_system_abort(msg); } -Config::Wrap Config::get(const String &s) +Config::Wrap Config::get(const char *s, size_t s_len) { ASSERT_MAIN_THREAD(); if (!this->is()) { - abort_on_object_get_failure(this, s.c_str()); + abort_on_object_get_failure(this, s); } - Wrap wrap(value.val.o.get(s)); + Wrap wrap(value.val.o.get(s, s_len)); return wrap; } -const Config::ConstWrap Config::get(const String &s) const +const Config::ConstWrap Config::get(const char *s, size_t s_len) const { ASSERT_MAIN_THREAD(); if (!this->is()) { - abort_on_object_get_failure(this, s.c_str()); + abort_on_object_get_failure(this, s); } - ConstWrap wrap(value.val.o.get(s)); + ConstWrap wrap(value.val.o.get(s, s_len)); return wrap; } +Config::Wrap Config::get(const String &s) +{ + return get(s.c_str(), s.length()); +} + +const Config::ConstWrap Config::get(const String &s) const +{ + return get(s.c_str(), s.length()); +} + [[gnu::noinline]] [[gnu::noreturn]] static void abort_on_array_get_failure(const Config *conf, size_t i) diff --git a/software/src/config.h b/software/src/config.h index 44bd14dae..1a4e377b8 100644 --- a/software/src/config.h +++ b/software/src/config.h @@ -241,8 +241,8 @@ struct Config { static Slot *allocSlotBuf(size_t elements); static void freeSlotBuf(Slot *buf); - Config *get(const String &s); - const Config *get(const String &s) const; + Config *get(const char *s, size_t s_len); + const Config *get(const char *s, size_t s_len) const; const Slot *getSlot() const; Slot *getSlot(); @@ -537,7 +537,7 @@ struct Config { static ConfigRoot *Confirm(); // Just for convenience. - static String ConfirmKey(); + static const char *ConfirmKey() {return Config::confirm_key;} static constexpr const char *confirm_key = "do_i_know_what_i_am_doing"; static Config Uint8(uint8_t u); @@ -593,10 +593,24 @@ struct Config { const ConstWrap get() const; // for ConfObject + Wrap get(const char *s, size_t s_len = 0); + const ConstWrap get(const char *s, size_t s_len = 0) const; Wrap get(const String &s); const ConstWrap get(const String &s) const; // for ConfArray + Wrap get(int8_t ) = delete; + const ConstWrap get(int8_t ) const = delete; + Wrap get(int16_t) = delete; + const ConstWrap get(int16_t) const = delete; + Wrap get(long ) = delete; + const ConstWrap get(long ) const = delete; + inline Wrap get(int i) {return get(static_cast(i));} // These casts should be safe, as negative values become huge positive values, + inline const ConstWrap get(int i) const {return get(static_cast(i));} // and the nested get() performs an array bounds check. + inline Wrap get(uint8_t i) {return get(static_cast(i));} + inline const ConstWrap get(uint8_t i) const {return get(static_cast(i));} + inline Wrap get(uint16_t i) {return get(static_cast(i));} + inline const ConstWrap get(uint16_t i) const {return get(static_cast(i));} Wrap get(size_t i); const ConstWrap get(size_t i) const; Wrap add(); diff --git a/software/src/config/conf_object.cpp b/software/src/config/conf_object.cpp index c2869c32a..47ce062a9 100644 --- a/software/src/config/conf_object.cpp +++ b/software/src/config/conf_object.cpp @@ -19,6 +19,8 @@ #include "config/private.h" +#include "event_log_prefix.h" +#include "main_dependencies.h" #include "tools/memory.h" bool Config::ConfObject::slotEmpty(size_t i) @@ -45,44 +47,68 @@ static void abort_on_key_not_found(const char *needle) esp_system_abort(msg); } -Config *Config::ConfObject::get(const String &needle) +Config *Config::ConfObject::get(const char *needle, size_t needle_len) { const auto *slot = this->getSlot(); const auto schema = slot->schema; const auto size = schema->length; + const auto keys = schema->keys; - const auto needle_length = needle.length(); - const auto needle_cstr = needle.c_str(); + if (string_is_in_rodata(needle)) { + for (size_t i = 0; i < size; ++i) { + if (keys[i].val == needle) { // Address comparison, not string comparison + return &slot->values[i]; + } + } + + logger.printfln("Key '%s' in rodata but not in keys.", needle); + } + + if (!needle_len) { + needle_len = strlen(needle); + } for (size_t i = 0; i < size; ++i) { - if (schema->keys[i].length != needle_length) + if (keys[i].length != needle_len) continue; - if (memcmp(schema->keys[i].val, needle_cstr, needle_length) == 0) + if (memcmp(keys[i].val, needle, needle_len) == 0) return &slot->values[i]; } - abort_on_key_not_found(needle_cstr); + abort_on_key_not_found(needle); } -const Config *Config::ConfObject::get(const String &needle) const +const Config *Config::ConfObject::get(const char *needle, size_t needle_len) const { const auto *slot = this->getSlot(); const auto schema = slot->schema; const auto size = schema->length; + const auto keys = schema->keys; - const auto needle_length = needle.length(); - const auto needle_cstr = needle.c_str(); + if (string_is_in_rodata(needle)) { + for (size_t i = 0; i < size; ++i) { + if (keys[i].val == needle) { // Address comparison, not string comparison + return &slot->values[i]; + } + } + + logger.printfln("Key '%s' in rodata but not in keys.", needle); + } + + if (!needle_len) { + needle_len = strlen(needle); + } for (size_t i = 0; i < size; ++i) { - if (schema->keys[i].length != needle_length) + if (keys[i].length != needle_len) continue; - if (memcmp(schema->keys[i].val, needle_cstr, needle_length) == 0) + if (memcmp(keys[i].val, needle, needle_len) == 0) return &slot->values[i]; } - abort_on_key_not_found(needle_cstr); + abort_on_key_not_found(needle); } const Config::ConfObject::Slot *Config::ConfObject::getSlot() const { return &object_buf[idx]; }