-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Utilize allocate_at_least
#3712
Utilize allocate_at_least
#3712
Conversation
allocate_at_least
#3570allocate_at_least
Why did this only get reported now??
Co-Authored-By: A. Jiang <[email protected]>
Also fix an element lifetime bug in xstring
This comment was marked as resolved.
This comment was marked as resolved.
I'm speculatively mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicky things that we should defer to a cleanup PR to avoid resetting testing.
_NODISCARD_RAW_PTR_ALLOC _CONSTEXPR20 typename allocator_traits<_Alloc>::pointer _Allocate_at_least_helper( | ||
_Alloc& _Al, _CRT_GUARDOVERFLOW typename allocator_traits<_Alloc>::size_type& _Count) { | ||
#if _HAS_CXX23 | ||
auto [_Ptr, _Allocated] = allocator_traits<_Alloc>::allocate_at_least(_Al, _Count); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we _STL_ASSERT(_Allocated >= _Count);
here?
|
||
signalling_allocator() = default; | ||
|
||
template <typename U> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with line 32 (and because tests should follow product code conventions):
template <typename U> | |
template <class U> |
signalling_allocator(const signalling_allocator<U>&) {} | ||
|
||
T* allocate(size_t count) { | ||
T* const ptr = static_cast<T*>(malloc(count * sizeof(T))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We conventionally use auto
when the initializer is a cast to avoid repeating the type:
T* const ptr = static_cast<T*>(malloc(count * sizeof(T))); | |
const auto ptr = static_cast<T*>(malloc(count * sizeof(T))); |
friend bool operator==(const signalling_allocator&, const signalling_allocator&) = default; | ||
}; | ||
|
||
template <typename T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
template <typename T> | |
template <class T> |
Thanks for helping STL containers light up with this new allocator machinery! 💡 🎉 ⚙️ |
This reverts commit e37227e. This broke BitCoin in Microsoft's internal Real World Code (RWC) test suite. They publicy derive an allocator from `std::allocate`, implementing `allocate` and `deallocate` but not `allocate_at_least` (https://github.com/bitcoin/bitcoin/blob/f1b4975461364d5d40d2bfafc6b165dd5d7eec30/src/support/allocators/secure.h#L19-L56). When `vector` allocates memory with `std::allocator::allocate_at_least` and tries to free it with `secure_allocator::deallocate`, terrible things happen. We suspect this pattern is widespread, so we're reverting the change for now.
This reverts commit 8ad049e.
This reverts commit 8ad049e.
… std::allocator 07c59ed Don't derive secure_allocator from std::allocator (Casey Carter) Pull request description: Giving the C++ Standard Committee control of the public interface of your type means they will break it. C++23 adds a new `allocate_at_least` member to `std::allocator`. Very bad things happen when, say, `std::vector` uses `allocate_at_least` from `secure_allocator`'s base to allocate memory which it then tries to free with `secure_allocator::deallocate`. (Discovered by microsoft/STL#3712, which will be reverted by microsoft/STL#3819 before it ships.) ACKs for top commit: jonatack: re-ACK 07c59ed no change since my previous ACK apart from squashing the commits achow101: ACK 07c59ed john-moffett: ACK 07c59ed Reviewed and tested. Performance appears unaffected in my environment. Tree-SHA512: 23606c40414d325f5605a9244d4dd50907fdf5f2fbf70f336accb3a2cb98baa8acd2972f46eab1b7fdec1d28a843a96b06083cd2d09791cda7c90ee218e5bbd5
…locator 07c59ed Don't derive secure_allocator from std::allocator (Casey Carter) Pull request description: Giving the C++ Standard Committee control of the public interface of your type means they will break it. C++23 adds a new `allocate_at_least` member to `std::allocator`. Very bad things happen when, say, `std::vector` uses `allocate_at_least` from `secure_allocator`'s base to allocate memory which it then tries to free with `secure_allocator::deallocate`. (Discovered by microsoft/STL#3712, which will be reverted by microsoft/STL#3819 before it ships.) ACKs for top commit: jonatack: re-ACK 07c59ed no change since my previous ACK apart from squashing the commits achow101: ACK 07c59ed john-moffett: ACK 07c59ed Reviewed and tested. Performance appears unaffected in my environment. Tree-SHA512: 23606c40414d325f5605a9244d4dd50907fdf5f2fbf70f336accb3a2cb98baa8acd2972f46eab1b7fdec1d28a843a96b06083cd2d09791cda7c90ee218e5bbd5
Considerations
shrink_to_fit
I have opted to not utilize
allocate_at_least
forshrink_to_fit
(for vectors and strings), as overallocation in combination with repeated calls toshrink_to_fit
would result in continuous reallocations without changing the capacity (worst case)._Hash_vec
The internally used type
_Hash_vec
fromxhash
uses a similar system differentiating size from capacity as other containers, but documentation seems to suggest that "This implementation never has capacity() differ from size(), but the previous implementation could" (l. 281). I have assumed this is by design, if not, it can be adjusted similarly to the allocations elsewhere (probably in combination with adjusting that comment).Tests
I think the existing tests passing is probably good enough to ascertain that no functionality is impeded, although I will be looking into writing tests to ascertain that the proper
allocate_at_least
method is called.Closes #3570