From 8dc8cf8019339b15a00ac244ce36aa387a23bbb9 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Mon, 27 May 2019 09:25:05 -0400 Subject: [PATCH 1/3] Common/HttpRequest: std::move callback in constructor std::function is allowed to heap allocate in order to hold any necessary bound data in order to execute properly (e.g. lambdas with captures), so this avoids unnecessary reallocating. --- Source/Core/Common/HttpRequest.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Source/Core/Common/HttpRequest.cpp b/Source/Core/Common/HttpRequest.cpp index 8098e315bd..f0da40cdea 100644 --- a/Source/Core/Common/HttpRequest.cpp +++ b/Source/Core/Common/HttpRequest.cpp @@ -49,7 +49,7 @@ std::mutex HttpRequest::Impl::s_curl_was_inited_mutex; bool HttpRequest::Impl::s_curl_was_inited = false; HttpRequest::HttpRequest(std::chrono::milliseconds timeout_ms, ProgressCallback callback) - : m_impl(std::make_unique(timeout_ms, callback)) + : m_impl(std::make_unique(timeout_ms, std::move(callback))) { } @@ -107,7 +107,7 @@ int HttpRequest::Impl::CurlProgressCallback(Impl* impl, double dlnow, double dlt } HttpRequest::Impl::Impl(std::chrono::milliseconds timeout_ms, ProgressCallback callback) - : m_callback(callback) + : m_callback(std::move(callback)) { { std::lock_guard lk(s_curl_was_inited_mutex); From b15f595130c22c595464707a372132fc8e7ea72b Mon Sep 17 00:00:00 2001 From: Lioncash Date: Mon, 27 May 2019 09:31:13 -0400 Subject: [PATCH 2/3] Common/HttpRequest: Avoid unnecessary copies in loop in Fetch() Previously, every entry pair within the map would be copied. The reason for this is subtle. A std::map's internal entry type is defined as: std::pair but the loop was declaring it as: std::pair These two types aren't synonymous with one another and so the compiler is required to always perform a copy. Using structured bindings avoids this (as would plain auto or correcting the explicit type), while also allowing the use of more appropriate names compared to first and second. --- Source/Core/Common/HttpRequest.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Source/Core/Common/HttpRequest.cpp b/Source/Core/Common/HttpRequest.cpp index f0da40cdea..18ea402dc5 100644 --- a/Source/Core/Common/HttpRequest.cpp +++ b/Source/Core/Common/HttpRequest.cpp @@ -197,14 +197,14 @@ HttpRequest::Response HttpRequest::Impl::Fetch(const std::string& url, Method me curl_slist* list = nullptr; Common::ScopeGuard list_guard{[&list] { curl_slist_free_all(list); }}; - for (const std::pair>& header : headers) + for (const auto& [name, value] : headers) { - if (!header.second) - list = curl_slist_append(list, (header.first + ":").c_str()); - else if (header.second->empty()) - list = curl_slist_append(list, (header.first + ";").c_str()); + if (!value) + list = curl_slist_append(list, (name + ':').c_str()); + else if (value->empty()) + list = curl_slist_append(list, (name + ';').c_str()); else - list = curl_slist_append(list, (header.first + ": " + *header.second).c_str()); + list = curl_slist_append(list, (name + ": " + *value).c_str()); } curl_easy_setopt(m_curl.get(), CURLOPT_HTTPHEADER, list); From ab2adfb0a76e4e46a08f632cfb0525c40e7731f4 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Mon, 27 May 2019 09:46:54 -0400 Subject: [PATCH 3/3] Common/HttpRequest: Simplify cURL initialization std::call_once is guaranteed to execute the given callable object exactly once. This guarantee holds even if the function is called concurrently from several threads. Given that, we can replace the mutex and boolean flag with std::call_once and a std::once_flag to perform the same behavior. --- Source/Core/Common/HttpRequest.cpp | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/Source/Core/Common/HttpRequest.cpp b/Source/Core/Common/HttpRequest.cpp index 18ea402dc5..8929185a86 100644 --- a/Source/Core/Common/HttpRequest.cpp +++ b/Source/Core/Common/HttpRequest.cpp @@ -39,15 +39,11 @@ public: std::string EscapeComponent(const std::string& string); private: - static std::mutex s_curl_was_inited_mutex; - static bool s_curl_was_inited; + static inline std::once_flag s_curl_was_initialized; ProgressCallback m_callback; std::unique_ptr m_curl{nullptr, curl_easy_cleanup}; }; -std::mutex HttpRequest::Impl::s_curl_was_inited_mutex; -bool HttpRequest::Impl::s_curl_was_inited = false; - HttpRequest::HttpRequest(std::chrono::milliseconds timeout_ms, ProgressCallback callback) : m_impl(std::make_unique(timeout_ms, std::move(callback))) { @@ -109,14 +105,7 @@ int HttpRequest::Impl::CurlProgressCallback(Impl* impl, double dlnow, double dlt HttpRequest::Impl::Impl(std::chrono::milliseconds timeout_ms, ProgressCallback callback) : m_callback(std::move(callback)) { - { - std::lock_guard lk(s_curl_was_inited_mutex); - if (!s_curl_was_inited) - { - curl_global_init(CURL_GLOBAL_DEFAULT); - s_curl_was_inited = true; - } - } + std::call_once(s_curl_was_initialized, [] { curl_global_init(CURL_GLOBAL_DEFAULT); }); m_curl.reset(curl_easy_init()); if (!m_curl)