From a8efb53f4e571957671226271da20deb1e3832b8 Mon Sep 17 00:00:00 2001 From: Charles Milette Date: Fri, 7 Feb 2020 03:25:31 -0500 Subject: [PATCH] Reduce memory consumption of system_category().message() (#457) This avoids the allocation in _Winerror_message, and introduces a new version of it which uses FORMAT_MESSAGE_ALLOCATE_BUFFER to completely avoid overallocating memory Fixes #434 Co-authored-by: Billy O'Neal Co-authored-by: Casey Carter --- stl/CMakeLists.txt | 2 + stl/inc/__msvc_all_public_headers.hpp | 6 +- stl/inc/__msvc_system_error_abi.hpp | 41 +++++++++++++ stl/inc/system_error | 42 +++++++++----- .../stl_base/stl.files.settings.targets | 1 + stl/src/syserror.cpp | 37 +++++------- stl/src/syserror_import_lib.cpp | 58 +++++++++++++++++++ 7 files changed, 145 insertions(+), 42 deletions(-) create mode 100644 stl/inc/__msvc_system_error_abi.hpp create mode 100644 stl/src/syserror_import_lib.cpp diff --git a/stl/CMakeLists.txt b/stl/CMakeLists.txt index 5092acfdb..3a771a1a3 100644 --- a/stl/CMakeLists.txt +++ b/stl/CMakeLists.txt @@ -3,6 +3,7 @@ set(HEADERS ${CMAKE_CURRENT_LIST_DIR}/inc/__msvc_all_public_headers.hpp + ${CMAKE_CURRENT_LIST_DIR}/inc/__msvc_system_error_abi.hpp ${CMAKE_CURRENT_LIST_DIR}/inc/algorithm ${CMAKE_CURRENT_LIST_DIR}/inc/any ${CMAKE_CURRENT_LIST_DIR}/inc/array @@ -242,6 +243,7 @@ set(IMPLIB_SOURCES ${CMAKE_CURRENT_LIST_DIR}/src/nothrow.cpp ${CMAKE_CURRENT_LIST_DIR}/src/parallel_algorithms.cpp ${CMAKE_CURRENT_LIST_DIR}/src/sharedmutex.cpp + ${CMAKE_CURRENT_LIST_DIR}/src/syserror_import_lib.cpp ${CMAKE_CURRENT_LIST_DIR}/src/vector_algorithms.cpp ) diff --git a/stl/inc/__msvc_all_public_headers.hpp b/stl/inc/__msvc_all_public_headers.hpp index d06a53159..cb3a06c6d 100644 --- a/stl/inc/__msvc_all_public_headers.hpp +++ b/stl/inc/__msvc_all_public_headers.hpp @@ -6,8 +6,8 @@ // // This file may be changed, renamed, or removed at any time. -#ifndef _MSVC_ALL_PUBLIC_HEADERS_HPP -#define _MSVC_ALL_PUBLIC_HEADERS_HPP +#ifndef __MSVC_ALL_PUBLIC_HEADERS_HPP +#define __MSVC_ALL_PUBLIC_HEADERS_HPP #pragma warning(push) #pragma warning(1 : 4668) // 'MEOW' is not defined as a preprocessor macro, replacing with '0' for '#if/#elif' @@ -211,4 +211,4 @@ #pragma warning(pop) -#endif // _MSVC_ALL_PUBLIC_HEADERS_HPP +#endif // __MSVC_ALL_PUBLIC_HEADERS_HPP diff --git a/stl/inc/__msvc_system_error_abi.hpp b/stl/inc/__msvc_system_error_abi.hpp new file mode 100644 index 000000000..d6744adf1 --- /dev/null +++ b/stl/inc/__msvc_system_error_abi.hpp @@ -0,0 +1,41 @@ +// __msvc_system_error_abi.hpp internal header (core) + +// Copyright (c) Microsoft Corporation. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#pragma once +#ifndef __MSVC_SYSTEM_ERROR_ABI_HPP +#define __MSVC_SYSTEM_ERROR_ABI_HPP +#include +#if _STL_COMPILER_PREPROCESSOR +#include + +#pragma pack(push, _CRT_PACKING) +#pragma warning(push, _STL_WARNING_LEVEL) +#pragma warning(disable : _STL_DISABLED_WARNINGS) +_STL_DISABLE_CLANG_WARNINGS +#pragma push_macro("new") +#undef new + +#ifdef _M_CEE_PURE +#define __CLRCALL_PURE_OR_STDCALL __clrcall +#else +#define __CLRCALL_PURE_OR_STDCALL __stdcall +#endif + +_EXTERN_C +_NODISCARD size_t __CLRCALL_PURE_OR_STDCALL __std_get_string_size_without_trailing_whitespace( + const char* _Str, size_t _Size) noexcept; + +_NODISCARD size_t __CLRCALL_PURE_OR_STDCALL __std_system_error_allocate_message( + unsigned long _Message_id, char** _Ptr_str) noexcept; +void __CLRCALL_PURE_OR_STDCALL __std_system_error_deallocate_message(char* _Str) noexcept; +_END_EXTERN_C + +#pragma pop_macro("new") +_STL_RESTORE_CLANG_WARNINGS +#pragma warning(pop) +#pragma pack(pop) + +#endif // _STL_COMPILER_PREPROCESSOR +#endif // __MSVC_SYSTEM_ERROR_ABI_HPP diff --git a/stl/inc/system_error b/stl/inc/system_error index 6b4e54f89..9c2e17534 100644 --- a/stl/inc/system_error +++ b/stl/inc/system_error @@ -8,9 +8,10 @@ #define _SYSTEM_ERROR_ #include #if _STL_COMPILER_PREPROCESSOR +#include <__msvc_system_error_abi.hpp> #include -#include // for strerror -#include // for runtime_error +#include +#include #include #include @@ -432,8 +433,21 @@ protected: _CRTIMP2_PURE const char* __CLRCALL_PURE_OR_CDECL _Syserror_map(int); _CRTIMP2_PURE int __CLRCALL_PURE_OR_CDECL _Winerror_map(int); -_CRTIMP2_PURE unsigned long __CLRCALL_PURE_OR_CDECL _Winerror_message( - unsigned long _Message_id, char* _Narrow, unsigned long _Size); + +struct _System_error_message { + char* _Str; + size_t _Length; + + explicit _System_error_message(const unsigned long _Ec) noexcept + : _Str(nullptr), _Length(_CSTD __std_system_error_allocate_message(_Ec, &_Str)) {} + + _System_error_message(const _System_error_message&) = delete; + _System_error_message& operator=(const _System_error_message&) = delete; + + ~_System_error_message() { + _CSTD __std_system_error_deallocate_message(_Str); + } +}; // CLASS _Generic_error_category class _Generic_error_category : public error_category { // categorize a generic error @@ -464,7 +478,9 @@ public: _NODISCARD virtual string message(int _Errcode) const override { if (_Errcode == static_cast(io_errc::stream)) { - return "iostream stream error"; + static constexpr char _Iostream_error[] = "iostream stream error"; + constexpr size_t _Iostream_error_length = sizeof(_Iostream_error) - 1; // TRANSITION, DevCom-906503 + return string(_Iostream_error, _Iostream_error_length); } else { return _Generic_error_category::message(_Errcode); } @@ -483,18 +499,14 @@ public: } _NODISCARD virtual string message(int _Errcode) const override { - const unsigned long _Size = 32767; - string _Narrow(_Size, '\0'); - - const unsigned long _Val = _Winerror_message(static_cast(_Errcode), &_Narrow[0], _Size); - if (_Val == 0) { - _Narrow = "unknown error"; + const _System_error_message _Msg(static_cast(_Errcode)); + if (_Msg._Length == 0) { + static constexpr char _Unknown_error[] = "unknown error"; + constexpr size_t _Unknown_error_length = sizeof(_Unknown_error) - 1; // TRANSITION, DevCom-906503 + return string(_Unknown_error, _Unknown_error_length); } else { - _Narrow.resize(_Val); + return string(_Msg._Str, _Msg._Length); } - - _Narrow.shrink_to_fit(); - return _Narrow; } _NODISCARD virtual error_condition default_error_condition(int _Errval) const noexcept override { diff --git a/stl/msbuild/stl_base/stl.files.settings.targets b/stl/msbuild/stl_base/stl.files.settings.targets index 3349f56ca..8a2158bd9 100644 --- a/stl/msbuild/stl_base/stl.files.settings.targets +++ b/stl/msbuild/stl_base/stl.files.settings.targets @@ -173,6 +173,7 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception $(CrtRoot)\github\stl\src\nothrow.cpp; $(CrtRoot)\github\stl\src\parallel_algorithms.cpp; $(CrtRoot)\github\stl\src\sharedmutex.cpp; + $(CrtRoot)\github\stl\src\syserror_import_lib.cpp; $(CrtRoot)\github\stl\src\vector_algorithms.cpp; "> nativecpp diff --git a/stl/src/syserror.cpp b/stl/src/syserror.cpp index 05408966e..1abfbb169 100644 --- a/stl/src/syserror.cpp +++ b/stl/src/syserror.cpp @@ -3,19 +3,24 @@ // system_error message mapping -#include -#include #include #include +// TRANSITION, MSBuild +// MSBuild has a hard requirement against including the same file in both a DLL and its import lib, so we include +// the import lib .cpp here to make those functions available for internal use by other parts of our DLL. +#ifdef _DLL +#include "syserror_import_lib.cpp" +#endif + _STD_BEGIN struct _Win_errtab_t { // maps Windows error to Posix error int _Windows; errc _Posix; }; -static const _Win_errtab_t _Win_errtab[] = { +static constexpr _Win_errtab_t _Win_errtab[] = { // table of Windows/Posix pairs {ERROR_ACCESS_DENIED, errc::permission_denied}, {ERROR_ALREADY_EXISTS, errc::file_exists}, @@ -108,31 +113,15 @@ _CRTIMP2_PURE int __CLRCALL_PURE_OR_CDECL _Winerror_map(int _Errcode) { return 0; } +// TRANSITION, ABI: _Winerror_message() is preserved for binary compatibility _CRTIMP2_PURE unsigned long __CLRCALL_PURE_OR_CDECL _Winerror_message( unsigned long _Message_id, char* _Narrow, unsigned long _Size) { // convert to name of Windows error, return 0 for failure, otherwise return number of chars written // pre: _Size < INT_MAX - const unique_ptr _Wide(new wchar_t[_Size]); // not using make_unique because we want default-init - const auto _First = _Wide.get(); - unsigned long _Wide_chars = - FormatMessageW(FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS, 0, _Message_id, 0, _First, _Size, 0); + const unsigned long _Chars = FormatMessageA( + FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS, nullptr, _Message_id, 0, _Narrow, _Size, nullptr); - // remove any trailing whitespace - while (_Wide_chars != 0) { - switch (_First[--_Wide_chars]) { - case L' ': - case L'\n': - case L'\r': - case L'\t': - case L'\0': - continue; - default: - return static_cast( - WideCharToMultiByte(CP_ACP, 0, _First, _Wide_chars + 1, _Narrow, _Size, 0, 0)); - } - } - - return 0; + return static_cast(_CSTD __std_get_string_size_without_trailing_whitespace(_Narrow, _Chars)); } struct _Sys_errtab_t { // maps error_code to NTBS @@ -140,7 +129,7 @@ struct _Sys_errtab_t { // maps error_code to NTBS const char* _Name; }; -static const _Sys_errtab_t _Sys_errtab[] = { +static constexpr _Sys_errtab_t _Sys_errtab[] = { // table of Posix code/name pairs {errc::address_family_not_supported, "address family not supported"}, {errc::address_in_use, "address in use"}, diff --git a/stl/src/syserror_import_lib.cpp b/stl/src/syserror_import_lib.cpp new file mode 100644 index 000000000..8f8046d48 --- /dev/null +++ b/stl/src/syserror_import_lib.cpp @@ -0,0 +1,58 @@ +// Copyright (c) Microsoft Corporation. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +// This must be as small as possible, because its contents are +// injected into the msvcprt.lib and msvcprtd.lib import libraries. +// Do not include or define anything else here. +// In particular, basic_string must not be included here. + +#include <__msvc_system_error_abi.hpp> + +#include + +namespace { + struct _Whitespace_bitmap_t { + bool _Is_whitespace[256]; + + constexpr _Whitespace_bitmap_t() noexcept : _Is_whitespace{} { + _Is_whitespace[' '] = true; + _Is_whitespace['\n'] = true; + _Is_whitespace['\r'] = true; + _Is_whitespace['\t'] = true; + _Is_whitespace['\0'] = true; + } + + _NODISCARD constexpr bool _Test(const char _Ch) const noexcept { + return _Is_whitespace[static_cast(_Ch)]; + } + }; + + constexpr _Whitespace_bitmap_t _Whitespace_bitmap; +} // unnamed namespace + +_EXTERN_C +_NODISCARD size_t __CLRCALL_PURE_OR_STDCALL __std_get_string_size_without_trailing_whitespace( + const char* const _Str, size_t _Size) noexcept { + while (_Size != 0 && _Whitespace_bitmap._Test(_Str[_Size - 1])) { + --_Size; + } + + return _Size; +} + +_NODISCARD size_t __CLRCALL_PURE_OR_STDCALL __std_system_error_allocate_message( + const unsigned long _Message_id, char** const _Ptr_str) noexcept { + // convert to name of Windows error, return 0 for failure, otherwise return number of chars in buffer + // __std_system_error_deallocate_message should be called even if 0 is returned + // pre: *_Ptr_str == nullptr + const unsigned long _Chars = + FormatMessageA(FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS, + nullptr, _Message_id, 0, reinterpret_cast(_Ptr_str), 0, nullptr); + + return _CSTD __std_get_string_size_without_trailing_whitespace(*_Ptr_str, _Chars); +} + +void __CLRCALL_PURE_OR_STDCALL __std_system_error_deallocate_message(char* const _Str) noexcept { + LocalFree(_Str); +} +_END_EXTERN_C