From b0263af5a8ba63121a5a2a18a83d6bd19a5f59d5 Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Wed, 3 Jun 2020 15:38:32 -0700 Subject: [PATCH] logd: remove faulty optimization TagNameKey contains a pointer to a std::string and a std::string_view, such it can both own a string or reference a different string. This is meant to be a memory optimization. This, however, is actually a net pessimization. Due to these three below cases and typical usage pattern. Cases: 1) In the case where TagNameKey owns the string, 3 words are wasted, one for the pointer and two for the std::string_view. 2) In the case where TagNameKey references a short string, the same 3 words are wasted. This is because std::string has a feature called "Short String Optimization" that means std::string does not allocate for strings of sizes <= 10 on 32bit devices and <= 22 on 64bit devices. 3) In the case where TagNameKey references a longer string than the "Short String Optimization" limits, then this saves the string's length in bytes. Usage pattern: After boot on 32 bit cuttlefish, there were 679 entries for the first case, and only 69 in the third case. The 679 entries have an overhead of 679 * 3 * sizeof(void*) = 679 * 12 = 8148 bytes. The 69 strings in the third case have a total length and therefore savings of 1352 bytes. This is a net pessimization of 6796 bytes. I expect this same ratio to be similar throughout the device's uptime. This situation is worse on 64 bit devices. If cuttlefish were 64 bit, then there would have been only 18 items in the third case due to the larger "Short String Optimization" capacity, and the cost for the first case would have doubled. Given the above and the cost of maintaining extra code, this optimization is removed and a std::string is used as the hash table key instead. Test: logging unit tests Change-Id: I957c519b19edca4f7fc531d96b7144cf68bf4e16 --- logd/LogStatistics.cpp | 27 ++++++++++- logd/LogStatistics.h | 106 +++++------------------------------------ 2 files changed, 39 insertions(+), 94 deletions(-) diff --git a/logd/LogStatistics.cpp b/logd/LogStatistics.cpp index 625cbdec4..d49982af0 100644 --- a/logd/LogStatistics.cpp +++ b/logd/LogStatistics.cpp @@ -36,6 +36,30 @@ static const uint64_t monthSec = 31 * 24 * hourSec; std::atomic LogStatistics::SizesTotal; +static std::string TagNameKey(const LogStatisticsElement& element) { + if (IsBinary(element.log_id)) { + uint32_t tag = element.tag; + if (tag) { + const char* cp = android::tagToName(tag); + if (cp) { + return std::string(cp); + } + } + return android::base::StringPrintf("[%" PRIu32 "]", tag); + } + const char* msg = element.msg; + if (!msg) { + return "chatty"; + } + ++msg; + uint16_t len = element.msg_len; + len = (len <= 1) ? 0 : strnlen(msg, len - 1); + if (!len) { + return ""; + } + return std::string(msg, len); +} + LogStatistics::LogStatistics(bool enable_statistics) : enable(enable_statistics) { log_time now(CLOCK_REALTIME); log_id_for_each(id) { @@ -597,7 +621,8 @@ std::string TagNameEntry::formatHeader(const std::string& name, std::string("BYTES"), std::string("")); } -std::string TagNameEntry::format(const LogStatistics&, log_id_t, const TagNameKey& key_name) const { +std::string TagNameEntry::format(const LogStatistics&, log_id_t, + const std::string& key_name) const { std::string name; std::string pidstr; if (pid_ != (pid_t)-1) { diff --git a/logd/LogStatistics.h b/logd/LogStatistics.h index 9f085dca3..200c22810 100644 --- a/logd/LogStatistics.h +++ b/logd/LogStatistics.h @@ -397,93 +397,6 @@ class TagEntry : public EntryBaseDropped { uid_t uid_; }; -struct TagNameKey { - std::string* alloc; - std::string_view name; // Saves space if const char* - - explicit TagNameKey(const LogStatisticsElement& element) - : alloc(nullptr), name("", strlen("")) { - if (IsBinary(element.log_id)) { - uint32_t tag = element.tag; - if (tag) { - const char* cp = android::tagToName(tag); - if (cp) { - name = std::string_view(cp, strlen(cp)); - return; - } - } - alloc = new std::string( - android::base::StringPrintf("[%" PRIu32 "]", tag)); - if (!alloc) return; - name = std::string_view(alloc->c_str(), alloc->size()); - return; - } - const char* msg = element.msg; - if (!msg) { - name = std::string_view("chatty", strlen("chatty")); - return; - } - ++msg; - uint16_t len = element.msg_len; - len = (len <= 1) ? 0 : strnlen(msg, len - 1); - if (!len) { - name = std::string_view("", strlen("")); - return; - } - alloc = new std::string(msg, len); - if (!alloc) return; - name = std::string_view(alloc->c_str(), alloc->size()); - } - - explicit TagNameKey(TagNameKey&& rval) noexcept - : alloc(rval.alloc), name(rval.name.data(), rval.name.length()) { - rval.alloc = nullptr; - } - - explicit TagNameKey(const TagNameKey& rval) - : alloc(rval.alloc ? new std::string(*rval.alloc) : nullptr), - name(alloc ? alloc->data() : rval.name.data(), rval.name.length()) { - } - - ~TagNameKey() { - if (alloc) delete alloc; - } - - operator const std::string_view() const { - return name; - } - - const char* data() const { - return name.data(); - } - size_t length() const { - return name.length(); - } - - bool operator==(const TagNameKey& rval) const { - if (length() != rval.length()) return false; - if (length() == 0) return true; - return fastcmp(data(), rval.data(), length()) == 0; - } - bool operator!=(const TagNameKey& rval) const { - return !(*this == rval); - } - - size_t getAllocLength() const { - return alloc ? alloc->length() + 1 + sizeof(std::string) : 0; - } -}; - -// Hash for TagNameKey -template <> -struct std::hash - : public std::unary_function { - size_t operator()(const TagNameKey& __t) const noexcept { - if (!__t.length()) return 0; - return std::hash()(std::string_view(__t)); - } -}; - class TagNameEntry : public EntryBase { public: explicit TagNameEntry(const LogStatisticsElement& element) @@ -507,7 +420,7 @@ class TagNameEntry : public EntryBase { } std::string formatHeader(const std::string& name, log_id_t id) const; - std::string format(const LogStatistics& stat, log_id_t id, const TagNameKey& key_name) const; + std::string format(const LogStatistics& stat, log_id_t id, const std::string& key_name) const; private: pid_t tid_; @@ -515,7 +428,6 @@ class TagNameEntry : public EntryBase { uid_t uid_; }; -// Log Statistics class LogStatistics { friend UidEntry; friend PidEntry; @@ -556,7 +468,7 @@ class LogStatistics { tagTable_t securityTagTable GUARDED_BY(lock_); // global tag list - typedef LogHashtable tagNameTable_t; + typedef LogHashtable tagNameTable_t; tagNameTable_t tagNameTable; size_t sizeOf() const REQUIRES(lock_) { @@ -573,13 +485,21 @@ class LogStatistics { const char* name = it.second.name(); if (name) size += strlen(name) + 1; } - for (auto it : tagNameTable) size += it.first.getAllocLength(); + for (auto it : tagNameTable) { + size += sizeof(std::string); + size_t len = it.first.size(); + // Account for short string optimization: if the string's length is <= 22 bytes for 64 + // bit or <= 10 bytes for 32 bit, then there is no additional allocation. + if ((sizeof(std::string) == 24 && len > 22) || + (sizeof(std::string) != 24 && len > 10)) { + size += len; + } + } log_id_for_each(id) { size += uidTable[id].sizeOf(); size += uidTable[id].size() * sizeof(uidTable_t::iterator); size += pidSystemTable[id].sizeOf(); - size += - pidSystemTable[id].size() * sizeof(pidSystemTable_t::iterator); + size += pidSystemTable[id].size() * sizeof(pidSystemTable_t::iterator); } return size; }