<system_error>: Consider improving memory allocation of system_category().message()

[sylveon]

Currently, this method allocates 96kb of memory for each invocation by first allocating a 32k character string in the headers and then a 32k array of wchar_t in implementation files, then copying from the array to the string, and then shrink_to_fit does more allocation and copying.

STL/stl/inc/system_error

Lines 486 to 489 in 580e61a

	const unsigned long _Size = 32767;
	string _Narrow(_Size, '\0');
	const unsigned long _Val = _Winerror_message(static_cast<unsigned long>(_Errcode), &_Narrow[0], _Size);

STL/stl/src/syserror.cpp

Lines 113 to 119 in 31bed7a

	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<wchar_t[]> _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);

It seems like this behavior could be improved in vNext, by making use of FORMAT_MESSAGE_ALLOCATE_BUFFER to get exactly the right size required for the message. A hypothetical _Get_Winerror_message function could return a pair<wchar_t*, std::size_t>, the headers would then allocate a std::string with the right size, calling _Copy_Winerror_message to copy (and narrow) into the std::string's buffer, and then call _Free_Winerror_message to free the buffer allocated by FormatMessage.

Sidenote: is there a reason for not using FormatMessageA? This would remove WideCharToMultiByte's overhead

[BillyONeal]

is there a reason for not using FormatMessageA?

I'm not positive but I believe the STL is deployed in environments where that function is not available.

would remove WideCharToMultiByte's overhead

Not really, it would just move it elsewhere. (The inner encoding is always wchar_ts)

I think this is fixable without an ABI break by adding your new equivalent of _Winerror_message to the import library so I have not added the vNext tag.

[sylveon]

I believe it would still reduce user and library code size since the _Copy_Winerror_message step would not be required.

Also, say I want to implement this and I want to avoid leaking the FormatMessage buffer in the (unlikely) case std::string's construction throws, would it be ok to include <memory> in <system_error> for access to std::unique_ptr

[StephanTLavavej]

Throughput is always a concern, but I view <system_error> as an expensive header because it drags in <stdexcept>, <xstring>, and <xmemory>. The incremental cost of <memory> doesn't look that bad, so I think it's acceptable.

That said, I was responsible for the suboptimal allocation behavior here - I knew it was grabbing a lot of memory and figured it was tolerable - and I just finished checking in a half-megabyte of headers (see #172) - so if the other maintainers think <memory> is expensive, I will defer to them.

[CaseyCarter]

It may be ok to include <memory> in <system_error>, but it's not ok to include <Windows.h>. As @BillyONeal hinted earlier ("I think this is fixable without an ABI break by adding your new equivalent of _Winerror_message to the import library") the best fix for this is to implement another internal function in a CPP file (_Winerror_message2?), change the headers to call that function, and we'll have it compiled and inserted directly into the STL's import library so it's effectively statically linked.

[StephanTLavavej]

Note that all source files compiled into the STL's import library must obey special rules. In particular, they must not drag in string, vector, or any other types whose representation is affected by _ITERATOR_DEBUG_LEVEL. unique_ptr is probably okay.

[sylveon]

Yeah I only intended to include <memory> in <system_error> for something like

std::unique_ptr<wchar_t[], decltype(&_Winerror_message2_free)> ptr(alloc, &_Winerror_message2_free);

so that _Winerror_message2_free gets automatically called in the unlikely event that the constructor of std::string throws std::bad_alloc. I didn't intend to use <string> in the implementation code.

I'll work on it this weekend (and I plan to use FormatMessageA unless I'm told to absolutely not)

[StephanTLavavej]

I plan to use FormatMessageA unless I'm told to absolutely not

My checkin notes on March 1, 2015 said, with emphasis added:

system_error
576-577: _Winerror_message() wraps FormatMessageW() and WideCharToMultiByte(), in order to fix 1101599 "system_category().message() should use FormatMessage with FORMAT_MESSAGE_FROM_SYSTEM flag".

641-652: Fix 1101599. Previously, system_category()'s message() mapped Windows errors to Posix errors and then to strings, but users complained that this didn't handle Windows errors without Posix equivalents (of which there are many). Surprisingly (to me), there is a Windows API function to stringize Windows errors, so we just need to call it. Documentation: FormatMessage

I'd like to call FormatMessageA(), but Xbox (KernelX) doesn't have it, so I need to call FormatMessageW() and WideCharToMultiByte(). For the _Wide buffer within _Winerror_message(), MSDN says "This buffer cannot be larger than 64K bytes.", so I'm using 32K - 1 wchar_ts. For the _Narrow buffer, I'm using 32K - 1 chars. Obviously, WideCharToMultiByte() can convert N wchar_ts into N + More chars, but 32K - 1 should be way more than enough for all languages (I wrote a program to iterate through all error codes; the longest one in English was about 2K chars), and this simplifies the logic.

Like FormatMessageA(), _Winerror_message() returns 0 for failure, otherwise the number of narrow characters written (excluding the null terminator). So for failure, we return "unknown error" as usual. Otherwise, we resize to _Val, which trims the unused buffer and _Winerror_message()'s null terminator, leaving only basic_string's null terminator. Finally, I shrink_to_fit(), so users won't complain that they're stringifying a thousand errors and consuming 32 MB of memory.

stdcpp/syserror.cpp
The rest of the fix for 1101599 "system_category().message() should use FormatMessage with FORMAT_MESSAGE_FROM_SYSTEM flag". This just wraps FormatMessageW() and passes all of the flags that we need. Note that FORMAT_MESSAGE_IGNORE_INSERTS is necessary, to tell Windows to emit "%1" etc. in messages literally. Then it calls WideCharToMultiByte() and passes -1, which tells it to convert the null terminator. Therefore, _Bytes is either 0 for failure, or the total number of narrow characters written (including the null terminator), so we return _Bytes - 1 for success.

The good news is that since then, we've dropped the KernelX mode from our build system. (I'm told that Xbox devs use app-local copies of the OneCore version.) FormatMessageA is documented as "Windows XP [desktop apps | UWP apps]" and "Windows Server 2003 [desktop apps | UWP apps]" so it should be fine to use unconditionally now.

[BillyONeal]

It's also present in OneCoreSDK