From 6df65d7a5d9f11d886e285e49567499cea85dab3 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Mon, 3 Jun 2019 18:27:43 -0400 Subject: [PATCH 01/10] Common/Analytics: Default AnalyticsReportingBackend()'s destructor Stays consistent with AnalyticsReportBuilder. --- Source/Core/Common/Analytics.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/Core/Common/Analytics.h b/Source/Core/Common/Analytics.h index 4930ef4763..2c70c29de8 100644 --- a/Source/Core/Common/Analytics.h +++ b/Source/Core/Common/Analytics.h @@ -46,7 +46,7 @@ namespace Common class AnalyticsReportingBackend { public: - virtual ~AnalyticsReportingBackend() {} + virtual ~AnalyticsReportingBackend() = default; // Called from the AnalyticsReporter backend thread. virtual void Send(std::string report) = 0; }; From f813c4951a4e150b1f431085efdacf364b217dda Mon Sep 17 00:00:00 2001 From: Lioncash Date: Mon, 3 Jun 2019 18:30:52 -0400 Subject: [PATCH 02/10] Common/Analytics: Use deduction guides for std::lock_guard Avoids needing to hardcode the type of mutex. We can also make use of scoped_lock where two consecutive lock_guard instances are used. --- Source/Core/Common/Analytics.h | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/Source/Core/Common/Analytics.h b/Source/Core/Common/Analytics.h index 2c70c29de8..45245ea42e 100644 --- a/Source/Core/Common/Analytics.h +++ b/Source/Core/Common/Analytics.h @@ -61,7 +61,7 @@ public: AnalyticsReportBuilder(const AnalyticsReportBuilder& other) { *this = other; } AnalyticsReportBuilder(AnalyticsReportBuilder&& other) { - std::lock_guard lk(other.m_lock); + std::lock_guard lk{other.m_lock}; m_report = std::move(other.m_report); } @@ -69,8 +69,7 @@ public: { if (this != &other) { - std::lock_guard lk(m_lock); - std::lock_guard lk2(other.m_lock); + std::scoped_lock lk{m_lock, other.m_lock}; m_report = other.m_report; } return *this; @@ -81,7 +80,7 @@ public: { // Get before locking the object to avoid deadlocks with this += this. std::string other_report = other.Get(); - std::lock_guard lk(m_lock); + std::lock_guard lk{m_lock}; m_report += other_report; return *this; } @@ -89,7 +88,7 @@ public: template AnalyticsReportBuilder& AddData(const std::string& key, const T& value) { - std::lock_guard lk(m_lock); + std::lock_guard lk{m_lock}; AppendSerializedValue(&m_report, key); AppendSerializedValue(&m_report, value); return *this; @@ -98,7 +97,7 @@ public: template AnalyticsReportBuilder& AddData(const std::string& key, const std::vector& value) { - std::lock_guard lk(m_lock); + std::lock_guard lk{m_lock}; AppendSerializedValue(&m_report, key); AppendSerializedValueVector(&m_report, value); return *this; @@ -106,14 +105,14 @@ public: std::string Get() const { - std::lock_guard lk(m_lock); + std::lock_guard lk{m_lock}; return m_report; } // More efficient version of Get(). std::string Consume() { - std::lock_guard lk(m_lock); + std::lock_guard lk{m_lock}; return std::move(m_report); } From 58e2cd54866572b48e422733b3112c58f7cce3a7 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Mon, 3 Jun 2019 18:33:02 -0400 Subject: [PATCH 03/10] Common/Analytics: std::move std::string constructor parameter Allows calling code to move into the constructor, avoiding the creation of another string copy. --- Source/Core/Common/Analytics.cpp | 2 +- Source/Core/Common/Analytics.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Source/Core/Common/Analytics.cpp b/Source/Core/Common/Analytics.cpp index 4441e9a7df..ed48d15188 100644 --- a/Source/Core/Common/Analytics.cpp +++ b/Source/Core/Common/Analytics.cpp @@ -198,7 +198,7 @@ void StdoutAnalyticsBackend::Send(std::string report) HexDump(reinterpret_cast(report.data()), report.size()).c_str()); } -HttpAnalyticsBackend::HttpAnalyticsBackend(const std::string& endpoint) : m_endpoint(endpoint) +HttpAnalyticsBackend::HttpAnalyticsBackend(std::string endpoint) : m_endpoint(std::move(endpoint)) { } diff --git a/Source/Core/Common/Analytics.h b/Source/Core/Common/Analytics.h index 45245ea42e..dc1ab8c599 100644 --- a/Source/Core/Common/Analytics.h +++ b/Source/Core/Common/Analytics.h @@ -184,7 +184,7 @@ public: class HttpAnalyticsBackend : public AnalyticsReportingBackend { public: - HttpAnalyticsBackend(const std::string& endpoint); + explicit HttpAnalyticsBackend(std::string endpoint); ~HttpAnalyticsBackend() override; void Send(std::string report) override; From 7935c27b5286f532eac35079765820f853a52b0b Mon Sep 17 00:00:00 2001 From: Lioncash Date: Mon, 3 Jun 2019 18:35:26 -0400 Subject: [PATCH 04/10] Common/Analytics: Convert std::string overload into std::string_view Allows for both string types to be non-allocating. We can't remove the const char* overload in this case due to the fact that pointers can implicitly convert to bool, so if we removed the overload all const char arrays passed in would begin executing the bool overload instead of the string_view overload, which is definitely not what we want to occur. --- Source/Core/Common/Analytics.cpp | 8 ++++++-- Source/Core/Common/Analytics.h | 7 ++++--- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/Source/Core/Common/Analytics.cpp b/Source/Core/Common/Analytics.cpp index ed48d15188..63903767ba 100644 --- a/Source/Core/Common/Analytics.cpp +++ b/Source/Core/Common/Analytics.cpp @@ -76,15 +76,19 @@ AnalyticsReportBuilder::AnalyticsReportBuilder() m_report.push_back(WIRE_FORMAT_VERSION); } -void AnalyticsReportBuilder::AppendSerializedValue(std::string* report, const std::string& v) +void AnalyticsReportBuilder::AppendSerializedValue(std::string* report, std::string_view v) { AppendType(report, TypeId::STRING); AppendBytes(report, reinterpret_cast(v.data()), static_cast(v.size())); } +// We can't remove this overload despite the string_view overload due to the fact that +// pointers can implicitly convert to bool, so if we removed the overload, then all +// const char strings passed in would begin forwarding to the bool overload, +// which is definitely not what we want to occur. void AnalyticsReportBuilder::AppendSerializedValue(std::string* report, const char* v) { - AppendSerializedValue(report, std::string(v)); + AppendSerializedValue(report, std::string_view(v)); } void AnalyticsReportBuilder::AppendSerializedValue(std::string* report, bool v) diff --git a/Source/Core/Common/Analytics.h b/Source/Core/Common/Analytics.h index dc1ab8c599..a14150e21c 100644 --- a/Source/Core/Common/Analytics.h +++ b/Source/Core/Common/Analytics.h @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -86,7 +87,7 @@ public: } template - AnalyticsReportBuilder& AddData(const std::string& key, const T& value) + AnalyticsReportBuilder& AddData(std::string_view key, const T& value) { std::lock_guard lk{m_lock}; AppendSerializedValue(&m_report, key); @@ -95,7 +96,7 @@ public: } template - AnalyticsReportBuilder& AddData(const std::string& key, const std::vector& value) + AnalyticsReportBuilder& AddData(std::string_view key, const std::vector& value) { std::lock_guard lk{m_lock}; AppendSerializedValue(&m_report, key); @@ -117,7 +118,7 @@ public: } protected: - static void AppendSerializedValue(std::string* report, const std::string& v); + static void AppendSerializedValue(std::string* report, std::string_view v); static void AppendSerializedValue(std::string* report, const char* v); static void AppendSerializedValue(std::string* report, bool v); static void AppendSerializedValue(std::string* report, u64 v); From 2c2b9690bbb9e4cd5c6a27550adab53b95c3cac5 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Mon, 3 Jun 2019 19:25:01 -0400 Subject: [PATCH 05/10] Core/Analytics: Simplify static_assert We can just use a std::array here to simplify the size calculation. --- Source/Core/Core/Analytics.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Source/Core/Core/Analytics.cpp b/Source/Core/Core/Analytics.cpp index d83fb4762b..2b23d55c7a 100644 --- a/Source/Core/Core/Analytics.cpp +++ b/Source/Core/Core/Analytics.cpp @@ -1,5 +1,6 @@ #include "Core/Analytics.h" +#include #include #include #include @@ -138,12 +139,11 @@ void DolphinAnalytics::ReportGameStart() } // Keep in sync with enum class GameQuirk definition. -static const char* GAME_QUIRKS_NAMES[] = { +constexpr std::array GAME_QUIRKS_NAMES{ "icache-matters", "directly-reads-wiimote-input", }; -static_assert(sizeof(GAME_QUIRKS_NAMES) / sizeof(GAME_QUIRKS_NAMES[0]) == - static_cast(GameQuirk::COUNT), +static_assert(GAME_QUIRKS_NAMES.size() == static_cast(GameQuirk::COUNT), "Game quirks names and enum definition are out of sync."); void DolphinAnalytics::ReportGameQuirk(GameQuirk quirk) From a5caa95a4bbc0d457c9a036547781aaf8687e281 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Mon, 3 Jun 2019 19:35:26 -0400 Subject: [PATCH 06/10] Core/Analytics: Use std::string_view where applicable In these cases, the strings are treated as views anyways, so we can use them here to avoid potential allocations. --- Source/Core/Core/Analytics.cpp | 10 +++++----- Source/Core/Core/Analytics.h | 5 +++-- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/Source/Core/Core/Analytics.cpp b/Source/Core/Core/Analytics.cpp index 2b23d55c7a..d9e8835023 100644 --- a/Source/Core/Core/Analytics.cpp +++ b/Source/Core/Core/Analytics.cpp @@ -101,11 +101,11 @@ void DolphinAnalytics::GenerateNewIdentity() SConfig::GetInstance().SaveSettings(); } -std::string DolphinAnalytics::MakeUniqueId(const std::string& data) +std::string DolphinAnalytics::MakeUniqueId(std::string_view data) { - u8 digest[20]; - std::string input = m_unique_id + data; - mbedtls_sha1(reinterpret_cast(input.c_str()), input.size(), digest); + std::array digest; + const auto input = std::string{m_unique_id}.append(data); + mbedtls_sha1(reinterpret_cast(input.c_str()), input.size(), digest.data()); // Convert to hex string and truncate to 64 bits. std::string out; @@ -116,7 +116,7 @@ std::string DolphinAnalytics::MakeUniqueId(const std::string& data) return out; } -void DolphinAnalytics::ReportDolphinStart(const std::string& ui_type) +void DolphinAnalytics::ReportDolphinStart(std::string_view ui_type) { Common::AnalyticsReportBuilder builder(m_base_builder); builder.AddData("type", "dolphin-start"); diff --git a/Source/Core/Core/Analytics.h b/Source/Core/Core/Analytics.h index 194e1d1a18..c622469ee9 100644 --- a/Source/Core/Core/Analytics.h +++ b/Source/Core/Core/Analytics.h @@ -8,6 +8,7 @@ #include #include #include +#include #include #include "Common/Analytics.h" @@ -49,7 +50,7 @@ public: void GenerateNewIdentity(); // Reports a Dolphin start event. - void ReportDolphinStart(const std::string& ui_type); + void ReportDolphinStart(std::string_view ui_type); // Generates a base report for a "Game start" event. Also preseeds the // per-game base data. @@ -88,7 +89,7 @@ private: // Returns a unique ID derived on the global unique ID, hashed with some // report-specific data. This avoid correlation between different types of // events. - std::string MakeUniqueId(const std::string& data); + std::string MakeUniqueId(std::string_view data); // Unique ID. This should never leave the application. Only used derived // values created by MakeUniqueId. From 57454e90a7c9d507a979a79474d70554727422b7 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Mon, 3 Jun 2019 19:37:33 -0400 Subject: [PATCH 07/10] Core/Analytics: Make MakeUniqueId() a const member function This function doesn't modify instance state, so we can mark it as const. --- Source/Core/Core/Analytics.cpp | 2 +- Source/Core/Core/Analytics.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Source/Core/Core/Analytics.cpp b/Source/Core/Core/Analytics.cpp index d9e8835023..24df4af02b 100644 --- a/Source/Core/Core/Analytics.cpp +++ b/Source/Core/Core/Analytics.cpp @@ -101,7 +101,7 @@ void DolphinAnalytics::GenerateNewIdentity() SConfig::GetInstance().SaveSettings(); } -std::string DolphinAnalytics::MakeUniqueId(std::string_view data) +std::string DolphinAnalytics::MakeUniqueId(std::string_view data) const { std::array digest; const auto input = std::string{m_unique_id}.append(data); diff --git a/Source/Core/Core/Analytics.h b/Source/Core/Core/Analytics.h index c622469ee9..d21404a2ab 100644 --- a/Source/Core/Core/Analytics.h +++ b/Source/Core/Core/Analytics.h @@ -89,7 +89,7 @@ private: // Returns a unique ID derived on the global unique ID, hashed with some // report-specific data. This avoid correlation between different types of // events. - std::string MakeUniqueId(std::string_view data); + std::string MakeUniqueId(std::string_view data) const; // Unique ID. This should never leave the application. Only used derived // values created by MakeUniqueId. From ebf3de4d934b2de30e2d3f5d2e874cbecdf68e97 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Mon, 3 Jun 2019 19:41:41 -0400 Subject: [PATCH 08/10] Core/Analytics: Use std::lock_guard deduction guides Starting with C++17, the type of the mutex being locked no longer needs to be hardcoded into the type declaration. --- Source/Core/Core/Analytics.cpp | 4 ++-- Source/Core/Core/Analytics.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Source/Core/Core/Analytics.cpp b/Source/Core/Core/Analytics.cpp index 24df4af02b..ea661df38c 100644 --- a/Source/Core/Core/Analytics.cpp +++ b/Source/Core/Core/Analytics.cpp @@ -58,7 +58,7 @@ DolphinAnalytics::DolphinAnalytics() std::shared_ptr DolphinAnalytics::Instance() { - std::lock_guard lk(s_instance_mutex); + std::lock_guard lk{s_instance_mutex}; if (!s_instance) { s_instance.reset(new DolphinAnalytics()); @@ -68,7 +68,7 @@ std::shared_ptr DolphinAnalytics::Instance() void DolphinAnalytics::ReloadConfig() { - std::lock_guard lk(m_reporter_mutex); + std::lock_guard lk{m_reporter_mutex}; // Install the HTTP backend if analytics support is enabled. std::unique_ptr new_backend; diff --git a/Source/Core/Core/Analytics.h b/Source/Core/Core/Analytics.h index d21404a2ab..e711fe24bf 100644 --- a/Source/Core/Core/Analytics.h +++ b/Source/Core/Core/Analytics.h @@ -76,7 +76,7 @@ public: template void Send(T report) { - std::lock_guard lk(m_reporter_mutex); + std::lock_guard lk{m_reporter_mutex}; m_reporter.Send(report); } From ea9f8871703b802a1fc756433857338a68d22c1a Mon Sep 17 00:00:00 2001 From: Lioncash Date: Mon, 3 Jun 2019 19:44:18 -0400 Subject: [PATCH 09/10] Core/Analytics: Use inline on static member variables Starting with C++17, this allows for the same behavior as the existing code, but without the need to manually write the out-of-class definitions as well. --- Source/Core/Core/Analytics.cpp | 3 --- Source/Core/Core/Analytics.h | 4 ++-- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/Source/Core/Core/Analytics.cpp b/Source/Core/Core/Analytics.cpp index ea661df38c..ecf85be4ec 100644 --- a/Source/Core/Core/Analytics.cpp +++ b/Source/Core/Core/Analytics.cpp @@ -39,9 +39,6 @@ namespace constexpr const char* ANALYTICS_ENDPOINT = "https://analytics.dolphin-emu.org/report"; } // namespace -std::mutex DolphinAnalytics::s_instance_mutex; -std::shared_ptr DolphinAnalytics::s_instance; - #if defined(ANDROID) static std::function s_get_val_func; void DolphinAnalytics::AndroidSetGetValFunc(std::function func) diff --git a/Source/Core/Core/Analytics.h b/Source/Core/Core/Analytics.h index e711fe24bf..77faa3d80c 100644 --- a/Source/Core/Core/Analytics.h +++ b/Source/Core/Core/Analytics.h @@ -127,6 +127,6 @@ private: // Shared pointer in order to allow for multithreaded use of the instance and // avoid races at reinitialization time. - static std::mutex s_instance_mutex; - static std::shared_ptr s_instance; + static inline std::mutex s_instance_mutex; + static inline std::shared_ptr s_instance; }; From e59f72739a3b349f3e6b27450fb6b3c0a429194c Mon Sep 17 00:00:00 2001 From: Lioncash Date: Mon, 3 Jun 2019 19:45:23 -0400 Subject: [PATCH 10/10] Core/Analytics: Turn analytics URL into a C-string Less reading and also doesn't store an unnecessary pointer value in the executable, making it smaller by 8 bytes. --- Source/Core/Core/Analytics.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/Core/Core/Analytics.cpp b/Source/Core/Core/Analytics.cpp index ecf85be4ec..bc3a7e9872 100644 --- a/Source/Core/Core/Analytics.cpp +++ b/Source/Core/Core/Analytics.cpp @@ -36,7 +36,7 @@ namespace { -constexpr const char* ANALYTICS_ENDPOINT = "https://analytics.dolphin-emu.org/report"; +constexpr char ANALYTICS_ENDPOINT[] = "https://analytics.dolphin-emu.org/report"; } // namespace #if defined(ANDROID)