From 14adc2e631fdf476969f2bdcb415d895400e7af1 Mon Sep 17 00:00:00 2001 From: Billy O'Neal Date: Thu, 2 Aug 2018 23:45:08 -0700 Subject: [PATCH] Various micro perf fixes and cleanup found while implementing CN overrides so far (#818) * Microperf: Use lock_guard instead of unique_lock. * Bill hates lock_guard more. * Ref them connections. * Demacroize. * Commonize port and merge some CRLFs into adjacent string literals. * Add missing template keyword. * Avoid stringstream in constructing the proxy_str. * Remove unused forward declarations of some HTTP things. * Use NOMINMAX instead of undef min and max everywhere. * Bunch of inheritance hygiene. * What do you mean you want examples to compile? * More CR comments from Robert. * Add static. * Use existing to_string_t. --- Release/include/cpprest/asyncrt_utils.h | 8 +-- .../include/cpprest/details/web_utilities.h | 11 ---- Release/include/cpprest/streams.h | 2 +- Release/src/http/client/http_client_asio.cpp | 50 +++++++++---------- .../src/http/client/http_client_winhttp.cpp | 23 +++------ Release/src/http/client/http_client_winrt.cpp | 15 +++--- Release/src/http/common/http_msg.cpp | 3 -- .../src/http/listener/http_server_asio.cpp | 7 +-- .../src/http/listener/http_server_httpsys.cpp | 15 +++--- Release/src/json/json.cpp | 4 -- Release/src/pch/stdafx.h | 3 +- .../src/websockets/client/ws_client_wspp.cpp | 15 +++--- 12 files changed, 60 insertions(+), 96 deletions(-) diff --git a/Release/include/cpprest/asyncrt_utils.h b/Release/include/cpprest/asyncrt_utils.h index b5e61c9d14..3fbebd7d95 100644 --- a/Release/include/cpprest/asyncrt_utils.h +++ b/Release/include/cpprest/asyncrt_utils.h @@ -228,7 +228,7 @@ namespace conversions #if defined(__ANDROID__) template - inline std::string to_string(const T& t) + inline std::string to_string(const T t) { std::ostringstream os; os.imbue(std::locale::classic()); @@ -238,16 +238,16 @@ namespace conversions #endif template - inline utility::string_t to_string_t(T&& t) + inline utility::string_t to_string_t(const T t) { #ifdef _UTF16_STRINGS using std::to_wstring; - return to_wstring(std::forward(t)); + return to_wstring(t); #else #if !defined(__ANDROID__) using std::to_string; #endif - return to_string(std::forward(t)); + return to_string(t); #endif } diff --git a/Release/include/cpprest/details/web_utilities.h b/Release/include/cpprest/details/web_utilities.h index ba6416546e..f99a8aa933 100644 --- a/Release/include/cpprest/details/web_utilities.h +++ b/Release/include/cpprest/details/web_utilities.h @@ -14,17 +14,6 @@ namespace web { - -namespace http { namespace client { namespace details { -class winhttp_client; -class winrt_client; -class asio_context; -}}} -namespace websockets { namespace client { namespace details { -class winrt_callback_client; -class wspp_callback_client; -}}} - namespace details { diff --git a/Release/include/cpprest/streams.h b/Release/include/cpprest/streams.h index cb7cd4d0ea..d2923d3774 100644 --- a/Release/include/cpprest/streams.h +++ b/Release/include/cpprest/streams.h @@ -1748,7 +1748,7 @@ class type_parser parse(streams::streambuf buffer) { - return base::_parse_input,std::basic_string>(buffer, _accept_char, _extract_result); + return base::template _parse_input,std::basic_string>(buffer, _accept_char, _extract_result); } private: diff --git a/Release/src/http/client/http_client_asio.cpp b/Release/src/http/client/http_client_asio.cpp index 3bf1fcba23..67d56c1e8f 100644 --- a/Release/src/http/client/http_client_asio.cpp +++ b/Release/src/http/client/http_client_asio.cpp @@ -15,9 +15,6 @@ #include "stdafx.h" -#undef min -#undef max - #include "cpprest/asyncrt_utils.h" #include "../common/internal_http_helpers.h" @@ -83,7 +80,7 @@ using utility::conversions::details::to_string; using std::to_string; #endif -#define CRLF std::string("\r\n") +static const std::string CRLF("\r\n"); namespace web { namespace http { @@ -213,14 +210,15 @@ class asio_connection template void async_connect(const Iterator &begin, const Handler &handler) { - std::unique_lock lock(m_socket_lock); - if (!m_closed) - m_socket.async_connect(begin, handler); - else { - lock.unlock(); - handler(boost::asio::error::operation_aborted); - } + std::lock_guard lock(m_socket_lock); + if (!m_closed) { + m_socket.async_connect(begin, handler); + return; + } + } // unlock + + handler(boost::asio::error::operation_aborted); } template @@ -339,7 +337,7 @@ class asio_connection /// } /// /// -class asio_connection_pool : public std::enable_shared_from_this +class asio_connection_pool final : public std::enable_shared_from_this { public: asio_connection_pool() : m_pool_epoch_timer(crossplat::threadpool::shared_instance().service()) @@ -396,24 +394,25 @@ class asio_connection_pool : public std::enable_shared_from_this lock(self.m_lock); if (self.m_prev_epoch == self.m_epoch) { - self.m_connections.clear(); + connections.clear(); self.is_timer_running = false; return; } else { auto prev_epoch = self.m_prev_epoch; - auto erase_end = std::find_if(self.m_connections.begin(), self.m_connections.end(), + auto erase_end = std::find_if(connections.begin(), connections.end(), [prev_epoch](std::pair>& p) { return p.first > prev_epoch; }); - self.m_connections.erase(self.m_connections.begin(), erase_end); + connections.erase(connections.begin(), erase_end); start_epoch_interval(pool); } }); @@ -438,7 +437,7 @@ class asio_client final : public _http_client_communicator , m_start_with_ssl(base_uri().scheme() == U("https") && !this->client_config().proxy().is_specified()) {} - void send_request(const std::shared_ptr &request_ctx) override; + virtual void send_request(const std::shared_ptr &request_ctx) override; void release_connection(std::shared_ptr& conn) { @@ -468,7 +467,7 @@ class asio_client final : public _http_client_communicator const bool m_start_with_ssl; }; -class asio_context : public request_context, public std::enable_shared_from_this +class asio_context final : public request_context, public std::enable_shared_from_this { friend class asio_client; public: @@ -501,7 +500,7 @@ class asio_context : public request_context, public std::enable_shared_from_this return ctx; } - class ssl_proxy_tunnel : public std::enable_shared_from_this + class ssl_proxy_tunnel final : public std::enable_shared_from_this { public: ssl_proxy_tunnel(std::shared_ptr context, std::function)> ssl_tunnel_established) @@ -518,14 +517,15 @@ class asio_context : public request_context, public std::enable_shared_from_this const auto &base_uri = m_context->m_http_client->base_uri(); const auto &host = utility::conversions::to_utf8string(base_uri.host()); - const auto &port = base_uri.port(); + const int portRaw = base_uri.port(); + const int port = (portRaw != 0) ? portRaw : 443; std::ostream request_stream(&m_request); request_stream.imbue(std::locale::classic()); - request_stream << "CONNECT " << host << ":" << ((port != 0) ? port : 443) << " HTTP/1.1" << CRLF; - request_stream << "Host: " << host << ":" << ((port != 0) ? port : 443) << CRLF; - request_stream << "Proxy-Connection: Keep-Alive" << CRLF; + request_stream << "CONNECT " << host << ":" << port << " HTTP/1.1\r\n"; + request_stream << "Host: " << host << ":" << port << CRLF; + request_stream << "Proxy-Connection: Keep-Alive\r\n"; if(m_context->m_http_client->client_config().proxy().credentials().is_set()) { @@ -698,7 +698,7 @@ class asio_context : public request_context, public std::enable_shared_from_this request_stream.imbue(std::locale::classic()); const auto &host = utility::conversions::to_utf8string(base_uri.host()); - request_stream << utility::conversions::to_utf8string(method) << " " << utility::conversions::to_utf8string(encoded_resource) << " " << "HTTP/1.1" << CRLF; + request_stream << utility::conversions::to_utf8string(method) << " " << utility::conversions::to_utf8string(encoded_resource) << " " << "HTTP/1.1\r\n"; int port = base_uri.port(); @@ -766,7 +766,7 @@ class asio_context : public request_context, public std::enable_shared_from_this request_stream << utility::conversions::to_utf8string(::web::http::details::flatten_http_headers(ctx->m_request.headers())); request_stream << extra_headers; // Enforce HTTP connection keep alive (even for the old HTTP/1.0 protocol). - request_stream << "Connection: Keep-Alive" << CRLF << CRLF; + request_stream << "Connection: Keep-Alive\r\n\r\n"; // Start connection timeout timer. if (!ctx->m_timer.has_started()) @@ -1345,7 +1345,7 @@ class asio_context : public request_context, public std::enable_shared_from_this } else { - async_read_until_buffersize(octets + CRLF.size(), // + 2 for crlf + async_read_until_buffersize(octets + CRLF.size(), boost::bind(&asio_context::handle_chunk, shared_from_this(), boost::asio::placeholders::error, octets)); } } diff --git a/Release/src/http/client/http_client_winhttp.cpp b/Release/src/http/client/http_client_winhttp.cpp index f6d343c2b2..ce037a4954 100644 --- a/Release/src/http/client/http_client_winhttp.cpp +++ b/Release/src/http/client/http_client_winhttp.cpp @@ -23,9 +23,6 @@ #include #endif -#undef min -#undef max - namespace web { namespace http @@ -185,7 +182,7 @@ enum msg_body_type }; // Additional information necessary to track a WinHTTP request. -class winhttp_request_context : public request_context +class winhttp_request_context final : public request_context { public: @@ -248,7 +245,7 @@ class winhttp_request_context : public request_context std::shared_ptr m_self_reference; memory_holder m_body_data; - virtual void cleanup() + void cleanup() { if(m_request_handle != nullptr) { @@ -260,7 +257,7 @@ class winhttp_request_context : public request_context protected: - virtual void finish() + virtual void finish() override { request_context::finish(); assert(m_self_reference != nullptr); @@ -346,7 +343,7 @@ struct ie_proxy_config : WINHTTP_CURRENT_USER_IE_PROXY_CONFIG }; // WinHTTP client. -class winhttp_client : public _http_client_communicator +class winhttp_client final : public _http_client_communicator { public: winhttp_client(http::uri address, http_client_config client_config) @@ -502,17 +499,13 @@ class winhttp_client : public _http_client_communicator } else { + proxy_str = uri.host(); if (uri.port() > 0) { - utility::ostringstream_t ss; - ss.imbue(std::locale::classic()); - ss << uri.host() << _XPLATSTR(":") << uri.port(); - proxy_str = ss.str(); - } - else - { - proxy_str = uri.host(); + proxy_str.push_back(_XPLATSTR(':')); + proxy_str.append(::utility::conversions::details::to_string_t(uri.port())); } + proxy_name = proxy_str.c_str(); } } diff --git a/Release/src/http/client/http_client_winrt.cpp b/Release/src/http/client/http_client_winrt.cpp index e8ee50a018..5a0697b489 100644 --- a/Release/src/http/client/http_client_winrt.cpp +++ b/Release/src/http/client/http_client_winrt.cpp @@ -28,9 +28,6 @@ using namespace std; using namespace Platform; using namespace Microsoft::WRL; -#undef min -#undef max - namespace web { namespace http @@ -41,7 +38,7 @@ namespace details { // Additional information necessary to track a WinRT request. -class winrt_request_context : public request_context +class winrt_request_context final : public request_context { public: @@ -63,7 +60,7 @@ class winrt_request_context : public request_context }; // Implementation of IXMLHTTPRequest2Callback. -class HttpRequestCallback : +class HttpRequestCallback final : public RuntimeClass, IXMLHTTPRequest2Callback, FtmBase> { public: @@ -190,7 +187,7 @@ class HttpRequestCallback : /// read and write operations. The I/O will be done off the UI thread, so there is no risk /// of causing the UI to become unresponsive. /// -class IRequestStream +class IRequestStream final : public Microsoft::WRL::RuntimeClass, ISequentialStream> { public: @@ -279,7 +276,7 @@ class IRequestStream /// read and write operations. The I/O will be done off the UI thread, so there is no risk /// of causing the UI to become unresponsive. /// -class IResponseStream +class IResponseStream final : public Microsoft::WRL::RuntimeClass, ISequentialStream> { public: @@ -353,7 +350,7 @@ class IResponseStream }; // WinRT client. -class winrt_client : public _http_client_communicator +class winrt_client final : public _http_client_communicator { public: winrt_client(http::uri&& address, http_client_config&& client_config) @@ -379,7 +376,7 @@ class winrt_client : public _http_client_communicator protected: // Start sending request. - void send_request(_In_ const std::shared_ptr &request) + virtual void send_request(_In_ const std::shared_ptr &request) override { http_request &msg = request->m_request; auto winrt_context = std::static_pointer_cast(request); diff --git a/Release/src/http/common/http_msg.cpp b/Release/src/http/common/http_msg.cpp index ece2d08c1e..8f218cb913 100644 --- a/Release/src/http/common/http_msg.cpp +++ b/Release/src/http/common/http_msg.cpp @@ -13,9 +13,6 @@ #include "stdafx.h" #include "../common/internal_http_helpers.h" -#undef min -#undef max - using namespace web; using namespace utility; using namespace concurrency; diff --git a/Release/src/http/listener/http_server_asio.cpp b/Release/src/http/listener/http_server_asio.cpp index bef47126dc..13b813b858 100644 --- a/Release/src/http/listener/http_server_asio.cpp +++ b/Release/src/http/listener/http_server_asio.cpp @@ -14,9 +14,6 @@ */ #include "stdafx.h" -#undef min -#undef max - #include #include #include @@ -1085,7 +1082,7 @@ will_deref_and_erase_t asio_server_connection::cancel_sending_response_with_erro { auto * context = static_cast(response._get_server_context()); context->m_response_completed.set_exception(eptr); - + // always terminate the connection since error happens return finish_request_response(); } @@ -1096,7 +1093,7 @@ will_deref_and_erase_t asio_server_connection::handle_write_chunked_response(con { return handle_response_written(response, ec); } - + auto readbuf = response._get_impl()->instream().streambuf(); if (readbuf.is_eof()) { diff --git a/Release/src/http/listener/http_server_httpsys.cpp b/Release/src/http/listener/http_server_httpsys.cpp index 7d3cea203c..8da74d0083 100644 --- a/Release/src/http/listener/http_server_httpsys.cpp +++ b/Release/src/http/listener/http_server_httpsys.cpp @@ -22,9 +22,6 @@ #include "http_server_httpsys.h" #include "http_server_impl.h" -#undef min -#undef max - using namespace web; using namespace utility; using namespace concurrency; @@ -482,7 +479,7 @@ windows_request_context::~windows_request_context() // Bug is that task_completion_event accesses internal state after setting. // Workaround is to use a lock incurring additional synchronization, if can acquire // the lock then setting of the event has completed. - std::unique_lock lock(m_responseCompletedLock); + std::lock_guard lock(m_responseCompletedLock); // Add a task-based continuation so no exceptions thrown from the task go 'unobserved'. pplx::create_task(m_response_completed).then([](pplx::task t) @@ -759,7 +756,7 @@ void windows_request_context::init_response_callbacks(ShouldWaitForBody shouldWa catch (...) { // Should never get here, if we do there's a chance that a circular reference will cause leaks, - // or worse, undefined behaviour as we don't know who owns 'this' anymore + // or worse, undefined behaviour as we don't know who owns 'this' anymore _ASSERTE(false); m_response = http::http_response(status_codes::InternalError); } @@ -899,7 +896,7 @@ void windows_request_context::transmit_body() if ( !m_sending_in_chunks && !m_transfer_encoding ) { // We are done sending data. - std::unique_lock lock(m_responseCompletedLock); + std::lock_guard lock(m_responseCompletedLock); m_response_completed.set(); return; } @@ -1024,7 +1021,7 @@ void windows_request_context::send_response_body_io_completion(DWORD error_code, /// void windows_request_context::cancel_request_io_completion(DWORD, DWORD) { - std::unique_lock lock(m_responseCompletedLock); + std::lock_guard lock(m_responseCompletedLock); m_response_completed.set_exception(m_except_ptr); } @@ -1047,7 +1044,7 @@ void windows_request_context::cancel_request(std::exception_ptr except_ptr) if(error_code != NO_ERROR && error_code != ERROR_IO_PENDING) { CancelThreadpoolIo(pServer->m_threadpool_io); - std::unique_lock lock(m_responseCompletedLock); + std::lock_guard lock(m_responseCompletedLock); m_response_completed.set_exception(except_ptr); } } @@ -1059,4 +1056,4 @@ std::unique_ptr make_http_httpsys_server() }}}} -#endif +#endif diff --git a/Release/src/json/json.cpp b/Release/src/json/json.cpp index 04ddbe0fe4..e5a6089aea 100644 --- a/Release/src/json/json.cpp +++ b/Release/src/json/json.cpp @@ -13,9 +13,6 @@ #include "stdafx.h" -#undef min -#undef max - using namespace web; bool json::details::g_keep_json_object_unsorted = false; @@ -519,4 +516,3 @@ const web::json::details::json_error_category_impl& web::json::details::json_err #endif return instance; } - diff --git a/Release/src/pch/stdafx.h b/Release/src/pch/stdafx.h index 1532db2f54..93ad966509 100644 --- a/Release/src/pch/stdafx.h +++ b/Release/src/pch/stdafx.h @@ -20,6 +20,7 @@ #endif #ifdef _WIN32 +#define NOMINMAX #ifdef CPPREST_TARGET_XP #include #ifndef _WIN32_WINNT @@ -137,4 +138,4 @@ #if defined(__clang__) #pragma clang diagnostic pop -#endif +#endif diff --git a/Release/src/websockets/client/ws_client_wspp.cpp b/Release/src/websockets/client/ws_client_wspp.cpp index 66b85744f4..c670dd0a61 100644 --- a/Release/src/websockets/client/ws_client_wspp.cpp +++ b/Release/src/websockets/client/ws_client_wspp.cpp @@ -20,10 +20,6 @@ #include "ws_client_impl.h" -// These must be undef'ed before including websocketpp because it is not Windows.h safe. -#undef min -#undef max - // Force websocketpp to use C++ std::error_code instead of Boost. #define _WEBSOCKETPP_CPP11_SYSTEM_ERROR_ #if defined(_MSC_VER) @@ -134,22 +130,23 @@ class wspp_callback_client : public websocket_client_callback_impl, public std:: ~wspp_callback_client() CPPREST_NOEXCEPT { _ASSERTE(m_state < DESTROYED); - std::unique_lock lock(m_wspp_client_lock); + State localState; + { + std::lock_guard lock(m_wspp_client_lock); + localState = m_state; + } // Unlock the mutex so connect/close can use it. // Now, what states could we be in? - switch (m_state) { + switch (localState) { case DESTROYED: // This should be impossible std::abort(); case CREATED: - lock.unlock(); break; case CLOSED: case CONNECTING: case CONNECTED: case CLOSING: - // Unlock the mutex so connect/close can use it. - lock.unlock(); try { // This will do nothing in the already-connected case