Skip to content
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.

Sign up for GitHub

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

[libc++][string] Fixes shrink_to_fit. #97961

Merged
merged 3 commits into from
Jul 23, 2024
Merged

[libc++][string] Fixes shrink_to_fit. #97961

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0de62f1
[libc++][string] Fixes shrink_to_fit.
mordante Jul 6, 2024
7eaa7da
Address review comment and formatting.
mordante Jul 20, 2024
1bc85ab
Fixes asan annotations.
mordante Jul 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions libcxx/include/string
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3265,23 +3265,38 @@ basic_string<_CharT, _Traits, _Allocator>::__shrink_or_extend(size_type __target
__p = __get_long_pointer();
} else {
if (__target_capacity > __cap) {
// Extend
// - called from reserve should propagate the exception thrown.
auto __allocation = std::__allocate_at_least(__alloc(), __target_capacity + 1);
__new_data = __allocation.ptr;
__target_capacity = __allocation.count - 1;
} else {
// Shrink
// - called from shrink_to_fit should not throw.
// - called from reserve may throw but is not required to.
#ifndef _LIBCPP_HAS_NO_EXCEPTIONS
try {
#endif // _LIBCPP_HAS_NO_EXCEPTIONS
auto __allocation = std::__allocate_at_least(__alloc(), __target_capacity + 1);

#ifdef _LIBCPP_HAS_NO_EXCEPTIONS
if (__allocation.ptr == nullptr)
mordante marked this conversation as resolved.
Show resolved Hide resolved
return;
#endif // _LIBCPP_HAS_NO_EXCEPTIONS

// The Standard mandates shrink_to_fit() does not increase the capacity.
// With equal capacity keep the existing buffer. This avoids extra work
// due to swapping the elements.
if (__allocation.count - 1 > __target_capacity) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this here really assumes that we're only getting into this code path in shrink_to_fit. Otherwise, we may be discarding the larger block of memory we just allocated without really needing to. Would it make sense to instead rewrite shrink_to_fit to avoid using this __shrink_or_extend method which seems kinda convoluted?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would require quite a bit of copy pasting. Both resize and shrink_to_fit need to handle the SSO buffer part. IMO it's no issue to keep this in one function. I expect the growing buffer is very unlikely to happen with typical allocators. I also expect the shrinking reserve and shrink_to_fit are not very popular functions unless you know it will safe a considerable amount of memory.

__alloc_traits::deallocate(__alloc(), __allocation.ptr, __allocation.count);
return;
}
__new_data = __allocation.ptr;
__target_capacity = __allocation.count - 1;
#ifndef _LIBCPP_HAS_NO_EXCEPTIONS
} catch (...) {
return;
}
#else // _LIBCPP_HAS_NO_EXCEPTIONS
if (__new_data == nullptr)
return;
#endif // _LIBCPP_HAS_NO_EXCEPTIONS
}
__begin_lifetime(__new_data, __target_capacity + 1);
Expand Down
41 changes: 41 additions & 0 deletions libcxx/test/std/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,49 @@ TEST_CONSTEXPR_CXX20 bool test() {
return true;
}

#if TEST_STD_VER >= 23
std::size_t min_bytes = 1000;

template <typename T>
struct increasing_allocator {
using value_type = T;
increasing_allocator() = default;
template <typename U>
increasing_allocator(const increasing_allocator<U>&) noexcept {}
std::allocation_result<T*> allocate_at_least(std::size_t n) {
std::size_t allocation_amount = n * sizeof(T);
if (allocation_amount < min_bytes)
allocation_amount = min_bytes;
min_bytes += 1000;
return {static_cast<T*>(::operator new(allocation_amount)), allocation_amount / sizeof(T)};
}
T* allocate(std::size_t n) { return allocate_at_least(n).ptr; }
void deallocate(T* p, std::size_t) noexcept { ::operator delete(static_cast<void*>(p)); }
};

template <typename T, typename U>
bool operator==(increasing_allocator<T>, increasing_allocator<U>) {
return true;
}

// https://github.com/llvm/llvm-project/issues/95161
void test_increasing_allocator() {
std::basic_string<char, std::char_traits<char>, increasing_allocator<char>> s{
"String does not fit in the internal buffer"};
std::size_t capacity = s.capacity();
std::size_t size = s.size();
s.shrink_to_fit();
assert(s.capacity() <= capacity);
assert(s.size() == size);
LIBCPP_ASSERT(is_string_asan_correct(s));
}
#endif // TEST_STD_VER >= 23

int main(int, char**) {
test();
#if TEST_STD_VER >= 23
test_increasing_allocator();
#endif
#if TEST_STD_VER > 17
static_assert(test());
#endif
Expand Down
Loading