[libcxx-commits] [libcxx] [libc++] Eliminate extra allocations from `std::move(oss).str()` (PR #67294)
via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Sep 25 01:30:21 PDT 2023
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-libcxx
<details>
<summary>Changes</summary>
Add test coverage for the new behaviors, especially to verify that the returned string uses the correct allocator.
Migrated from https://reviews.llvm.org/D157776 — @<!-- -->philnik777 @<!-- -->pfusik
@<!-- -->ldionne @<!-- -->mordante
please take another look!
---
Patch is 29.00 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/67294.diff
11 Files Affected:
- (modified) libcxx/include/sstream (+5-5)
- (modified) libcxx/include/string (+15-6)
- (added) libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.allocator_propagation.pass.cpp (+165)
- (modified) libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.move.pass.cpp (+7)
- (added) libcxx/test/std/input.output/string.streams/ostringstream/ostringstream.members/str.allocator_propagation.pass.cpp (+137)
- (modified) libcxx/test/std/input.output/string.streams/ostringstream/ostringstream.members/str.move.pass.cpp (+7)
- (modified) libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.members/str.move.pass.cpp (+43)
- (modified) libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.members/str.pass.cpp (+34)
- (modified) libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.members/view.pass.cpp (+28)
- (added) libcxx/test/std/input.output/string.streams/stringstream/stringstream.members/str.allocator_propagation.pass.cpp (+165)
- (modified) libcxx/test/std/input.output/string.streams/stringstream/stringstream.members/str.move.pass.cpp (+7)
``````````diff
diff --git a/libcxx/include/sstream b/libcxx/include/sstream
index 40930df24c6d086..15b805b7a29fc76 100644
--- a/libcxx/include/sstream
+++ b/libcxx/include/sstream
@@ -399,12 +399,12 @@ public:
_LIBCPP_HIDE_FROM_ABI_SSTREAM string_type str() const & { return str(__str_.get_allocator()); }
_LIBCPP_HIDE_FROM_ABI_SSTREAM string_type str() && {
- string_type __result;
const basic_string_view<_CharT, _Traits> __view = view();
- if (!__view.empty()) {
- auto __pos = __view.data() - __str_.data();
- __result.assign(std::move(__str_), __pos, __view.size());
- }
+ typename string_type::size_type __pos = __view.empty() ? 0 : __view.data() - __str_.data();
+ // In C++23, this is just string_type(std::move(__str_), __pos, __view.size(), __str_.get_allocator());
+ // But we need something that works in C++20 also.
+ string_type __result(__str_.get_allocator());
+ __result.__move_assign(std::move(__str_), __pos, __view.size());
__str_.clear();
__init_buf_ptrs();
return __result;
diff --git a/libcxx/include/string b/libcxx/include/string
index 123e1d64f910ab7..d79601076cebcdc 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -983,12 +983,7 @@ public:
auto __len = std::min<size_type>(__n, __str.size() - __pos);
if (__alloc_traits::is_always_equal::value || __alloc == __str.__alloc()) {
- __r_.first() = __str.__r_.first();
- __str.__default_init();
-
- _Traits::move(data(), data() + __pos, __len);
- __set_size(__len);
- _Traits::assign(data()[__len], value_type());
+ __move_assign(std::move(__str), __pos, __len);
} else {
// Perform a copy because the allocators are not compatible.
__init(__str.data() + __pos, __len);
@@ -1333,6 +1328,20 @@ public:
return assign(__sv.data(), __sv.size());
}
+#if _LIBCPP_STD_VER >= 20
+ _LIBCPP_HIDE_FROM_ABI constexpr
+ void __move_assign(basic_string&& __str, size_type __pos, size_type __len) {
+ // Pilfer the allocation from __str.
+ // Precondition: __alloc == __str.__alloc()
+ __r_.first() = __str.__r_.first();
+ __str.__default_init();
+
+ _Traits::move(data(), data() + __pos, __len);
+ __set_size(__len);
+ _Traits::assign(data()[__len], value_type());
+ }
+#endif
+
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20
basic_string& assign(const basic_string& __str) { return *this = __str; }
#ifndef _LIBCPP_CXX03_LANG
diff --git a/libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.allocator_propagation.pass.cpp b/libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.allocator_propagation.pass.cpp
new file mode 100644
index 000000000000000..28cc2216bc233b2
--- /dev/null
+++ b/libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.allocator_propagation.pass.cpp
@@ -0,0 +1,165 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// TODO: Change to XFAIL once https://github.com/llvm/llvm-project/issues/40340 is fixed
+// UNSUPPORTED: availability-pmr-missing
+
+// This test ensures that we properly propagate allocators from istringstream's
+// inner string object to the new string returned from .str().
+// `str() const&` is specified to preserve the allocator (not copy the string).
+// `str() &&` isn't specified, but should preserve the allocator (move the string).
+
+#include <cassert>
+#include <memory>
+#include <memory_resource>
+#include <sstream>
+#include <string>
+#include <string_view>
+#include <type_traits>
+
+#include "make_string.h"
+#include "test_macros.h"
+
+template <class T>
+struct SocccAllocator {
+ using value_type = T;
+
+ int count_ = 0;
+ explicit SocccAllocator(int i) : count_(i) {}
+
+ template<class U>
+ SocccAllocator(const SocccAllocator<U>& a) : count_(a.count_) {}
+
+ T* allocate(std::size_t n) { return std::allocator<T>().allocate(n); }
+ void deallocate(T* p, std::size_t n) { std::allocator<T>().deallocate(p, n); }
+
+ SocccAllocator select_on_container_copy_construction() const {
+ return SocccAllocator(count_ + 1);
+ }
+
+ bool operator==(const SocccAllocator&) const { return true; }
+
+ using propagate_on_container_copy_assignment = std::false_type;
+ using propagate_on_container_move_assignment = std::false_type;
+ using propagate_on_container_swap = std::false_type;
+};
+
+template <class CharT>
+void test_soccc_behavior()
+{
+ using Alloc = SocccAllocator<CharT>;
+ using SS = std::basic_istringstream<CharT, std::char_traits<CharT>, Alloc>;
+ using S = std::basic_string<CharT, std::char_traits<CharT>, Alloc>;
+ {
+ SS ss = SS(std::ios_base::in, Alloc(10));
+
+ // [stringbuf.members]/6 specifies that the allocator is copied,
+ // not select_on_container_copy_construction'ed.
+ //
+ S copied = ss.str();
+ assert(copied.get_allocator().count_ == 10);
+ assert(ss.rdbuf()->get_allocator().count_ == 10);
+ assert(copied.empty());
+
+ assert(S(copied).get_allocator().count_ == 11);
+ // sanity-check that SOCCC does in fact work
+
+ // [stringbuf.members]/10 doesn't specify the allocator to use,
+ // but copying the allocator as-if-by moving the string makes sense.
+ //
+ S moved = std::move(ss).str();
+ assert(moved.get_allocator().count_ == 10);
+ assert(ss.rdbuf()->get_allocator().count_ == 10);
+ assert(moved.empty());
+ }
+}
+
+template<class CharT, class Base = std::basic_stringbuf<CharT, std::char_traits<CharT>, std::pmr::polymorphic_allocator<CharT>>>
+struct StringBuf : Base {
+ explicit StringBuf(std::pmr::memory_resource *mr) : Base(std::ios_base::in, mr) {}
+ void public_setg(int a, int b, int c) {
+ CharT *p = this->eback();
+ assert(this->view().data() == p);
+ this->setg(p + a, p + b, p + c);
+ assert(this->eback() == p + a);
+ assert(this->view().data() == p + a);
+ }
+};
+
+template <class CharT>
+void test_allocation_is_pilfered() {
+ using SS = std::basic_istringstream<CharT, std::char_traits<CharT>, std::pmr::polymorphic_allocator<CharT>>;
+ using S = std::pmr::basic_string<CharT>;
+ alignas(void*) char buf[80 * sizeof(CharT)];
+ const CharT *initial = MAKE_CSTRING(CharT, "a very long string that exceeds the small string optimization buffer length");
+ {
+ std::pmr::set_default_resource(std::pmr::null_memory_resource());
+ auto mr1 = std::pmr::monotonic_buffer_resource(buf, sizeof(buf), std::pmr::null_memory_resource());
+ SS ss = SS(S(initial, &mr1));
+ S s = std::move(ss).str();
+ assert(s == initial);
+ }
+ {
+ // Try moving-out-of a stringbuf whose view() is not the entire string.
+ // This is libc++'s behavior; libstdc++ doesn't allow such stringbufs to be created.
+ //
+ std::pmr::set_default_resource(std::pmr::null_memory_resource());
+ auto mr1 = std::pmr::monotonic_buffer_resource(buf, sizeof(buf), std::pmr::null_memory_resource());
+ auto src = StringBuf<CharT>(&mr1);
+ src.str(S(initial, &mr1));
+ src.public_setg(2, 6, 40);
+ SS ss(std::ios_base::in, &mr1);
+ *ss.rdbuf() = std::move(src);
+ LIBCPP_ASSERT(ss.view() == std::basic_string_view<CharT>(initial).substr(2, 38));
+ S s = std::move(ss).str();
+ LIBCPP_ASSERT(s == std::basic_string_view<CharT>(initial).substr(2, 38));
+ }
+}
+
+template <class CharT>
+void test_no_foreign_allocations() {
+ using SS = std::basic_istringstream<CharT, std::char_traits<CharT>, std::pmr::polymorphic_allocator<CharT>>;
+ using S = std::pmr::basic_string<CharT>;
+ const CharT *initial = MAKE_CSTRING(CharT, "a very long string that exceeds the small string optimization buffer length");
+ {
+ std::pmr::set_default_resource(std::pmr::null_memory_resource());
+ auto mr1 = std::pmr::monotonic_buffer_resource(std::pmr::new_delete_resource());
+ auto ss = SS(S(initial, &mr1));
+ assert(ss.rdbuf()->get_allocator().resource() == &mr1);
+
+ // [stringbuf.members]/6 specifies that the result of `str() const &`
+ // does NOT use the default allocator; it uses the original allocator.
+ //
+ S copied = ss.str();
+ assert(copied.get_allocator().resource() == &mr1);
+ assert(ss.rdbuf()->get_allocator().resource() == &mr1);
+ assert(copied == initial);
+
+ // [stringbuf.members]/10 doesn't specify the allocator to use,
+ // but copying the allocator as-if-by moving the string makes sense.
+ //
+ S moved = std::move(ss).str();
+ assert(moved.get_allocator().resource() == &mr1);
+ assert(ss.rdbuf()->get_allocator().resource() == &mr1);
+ assert(moved == initial);
+ }
+}
+
+int main(int, char**) {
+ test_soccc_behavior<char>();
+ test_allocation_is_pilfered<char>();
+ test_no_foreign_allocations<char>();
+#ifndef TEST_HAS_NO_WIDE_CHARACTERS
+ test_soccc_behavior<wchar_t>();
+ test_allocation_is_pilfered<wchar_t>();
+ test_no_foreign_allocations<wchar_t>();
+#endif
+
+ return 0;
+}
diff --git a/libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.move.pass.cpp b/libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.move.pass.cpp
index 546f82166aaefa7..fb26fcff2655a13 100644
--- a/libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.move.pass.cpp
+++ b/libcxx/test/std/input.output/string.streams/istringstream/istringstream.members/str.move.pass.cpp
@@ -37,6 +37,13 @@ static void test() {
assert(s.empty());
assert(ss.view().empty());
}
+ {
+ std::basic_istringstream<CharT> ss(STR("a very long string that exceeds the small string optimization buffer length"));
+ const CharT *p = ss.view().data();
+ std::basic_string<CharT> s = std::move(ss).str();
+ assert(s.data() == p); // the allocation was pilfered
+ assert(ss.view().empty());
+ }
}
int main(int, char**) {
diff --git a/libcxx/test/std/input.output/string.streams/ostringstream/ostringstream.members/str.allocator_propagation.pass.cpp b/libcxx/test/std/input.output/string.streams/ostringstream/ostringstream.members/str.allocator_propagation.pass.cpp
new file mode 100644
index 000000000000000..2bc1abe571758d3
--- /dev/null
+++ b/libcxx/test/std/input.output/string.streams/ostringstream/ostringstream.members/str.allocator_propagation.pass.cpp
@@ -0,0 +1,137 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// TODO: Change to XFAIL once https://github.com/llvm/llvm-project/issues/40340 is fixed
+// UNSUPPORTED: availability-pmr-missing
+
+// This test ensures that we properly propagate allocators from ostringstream's
+// inner string object to the new string returned from .str().
+// `str() const&` is specified to preserve the allocator (not copy the string).
+// `str() &&` isn't specified, but should preserve the allocator (move the string).
+
+#include <cassert>
+#include <memory>
+#include <memory_resource>
+#include <sstream>
+#include <string>
+#include <type_traits>
+
+#include "make_string.h"
+#include "test_macros.h"
+
+template <class T>
+struct SocccAllocator {
+ using value_type = T;
+
+ int count_ = 0;
+ explicit SocccAllocator(int i) : count_(i) {}
+
+ template<class U>
+ SocccAllocator(const SocccAllocator<U>& a) : count_(a.count_) {}
+
+ T* allocate(std::size_t n) { return std::allocator<T>().allocate(n); }
+ void deallocate(T* p, std::size_t n) { std::allocator<T>().deallocate(p, n); }
+
+ SocccAllocator select_on_container_copy_construction() const {
+ return SocccAllocator(count_ + 1);
+ }
+
+ bool operator==(const SocccAllocator&) const { return true; }
+
+ using propagate_on_container_copy_assignment = std::false_type;
+ using propagate_on_container_move_assignment = std::false_type;
+ using propagate_on_container_swap = std::false_type;
+};
+
+template <class CharT>
+void test_soccc_behavior()
+{
+ using Alloc = SocccAllocator<CharT>;
+ using SS = std::basic_ostringstream<CharT, std::char_traits<CharT>, Alloc>;
+ using S = std::basic_string<CharT, std::char_traits<CharT>, Alloc>;
+ {
+ SS ss = SS(std::ios_base::out, Alloc(10));
+
+ // [stringbuf.members]/6 specifies that the allocator is copied,
+ // not select_on_container_copy_construction'ed.
+ //
+ S copied = ss.str();
+ assert(copied.get_allocator().count_ == 10);
+ assert(ss.rdbuf()->get_allocator().count_ == 10);
+ assert(copied.empty());
+
+ assert(S(copied).get_allocator().count_ == 11);
+ // sanity-check that SOCCC does in fact work
+
+ // [stringbuf.members]/10 doesn't specify the allocator to use,
+ // but copying the allocator as-if-by moving the string makes sense.
+ //
+ S moved = std::move(ss).str();
+ assert(moved.get_allocator().count_ == 10);
+ assert(ss.rdbuf()->get_allocator().count_ == 10);
+ assert(moved.empty());
+ }
+}
+
+template <class CharT>
+void test_allocation_is_pilfered() {
+ using SS = std::basic_ostringstream<CharT, std::char_traits<CharT>, std::pmr::polymorphic_allocator<CharT>>;
+ using S = std::pmr::basic_string<CharT>;
+ alignas(void*) char buf[80 * sizeof(CharT)];
+ const CharT *initial = MAKE_CSTRING(CharT, "a very long string that exceeds the small string optimization buffer length");
+ {
+ std::pmr::set_default_resource(std::pmr::null_memory_resource());
+ auto mr1 = std::pmr::monotonic_buffer_resource(buf, sizeof(buf), std::pmr::null_memory_resource());
+ SS ss = SS(S(initial, &mr1));
+ S s = std::move(ss).str();
+ assert(s == initial);
+ }
+}
+
+template <class CharT>
+void test_no_foreign_allocations() {
+ using SS = std::basic_ostringstream<CharT, std::char_traits<CharT>, std::pmr::polymorphic_allocator<CharT>>;
+ using S = std::pmr::basic_string<CharT>;
+ const CharT *initial = MAKE_CSTRING(CharT, "a very long string that exceeds the small string optimization buffer length");
+ {
+ std::pmr::set_default_resource(std::pmr::null_memory_resource());
+ auto mr1 = std::pmr::monotonic_buffer_resource(std::pmr::new_delete_resource());
+ auto ss = SS(S(initial, &mr1));
+ assert(ss.rdbuf()->get_allocator().resource() == &mr1);
+
+ // [stringbuf.members]/6 specifies that the result of `str() const &`
+ // does NOT use the default allocator; it uses the original allocator.
+ //
+ S copied = ss.str();
+ assert(copied.get_allocator().resource() == &mr1);
+ assert(ss.rdbuf()->get_allocator().resource() == &mr1);
+ assert(copied == initial);
+
+ // [stringbuf.members]/10 doesn't specify the allocator to use,
+ // but copying the allocator as-if-by moving the string makes sense.
+ //
+ S moved = std::move(ss).str();
+ assert(moved.get_allocator().resource() == &mr1);
+ assert(ss.rdbuf()->get_allocator().resource() == &mr1);
+ assert(moved == initial);
+ }
+}
+
+int main(int, char**) {
+ test_soccc_behavior<char>();
+ test_allocation_is_pilfered<char>();
+ test_no_foreign_allocations<char>();
+#ifndef TEST_HAS_NO_WIDE_CHARACTERS
+ test_soccc_behavior<wchar_t>();
+ test_allocation_is_pilfered<wchar_t>();
+ test_no_foreign_allocations<wchar_t>();
+#endif
+
+ return 0;
+}
diff --git a/libcxx/test/std/input.output/string.streams/ostringstream/ostringstream.members/str.move.pass.cpp b/libcxx/test/std/input.output/string.streams/ostringstream/ostringstream.members/str.move.pass.cpp
index 57f2384bae52c61..c51aeef4eae17d2 100644
--- a/libcxx/test/std/input.output/string.streams/ostringstream/ostringstream.members/str.move.pass.cpp
+++ b/libcxx/test/std/input.output/string.streams/ostringstream/ostringstream.members/str.move.pass.cpp
@@ -37,6 +37,13 @@ static void test() {
assert(s.empty());
assert(ss.view().empty());
}
+ {
+ std::basic_ostringstream<CharT> ss(STR("a very long string that exceeds the small string optimization buffer length"));
+ const CharT *p = ss.view().data();
+ std::basic_string<CharT> s = std::move(ss).str();
+ assert(s.data() == p); // the allocation was pilfered
+ assert(ss.view().empty());
+ }
}
int main(int, char**) {
diff --git a/libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.members/str.move.pass.cpp b/libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.members/str.move.pass.cpp
index 0f0f540a9c2474d..26c97045bdfe3e4 100644
--- a/libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.members/str.move.pass.cpp
+++ b/libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.members/str.move.pass.cpp
@@ -37,6 +37,48 @@ static void test() {
assert(s.empty());
assert(buf.view().empty());
}
+ {
+ std::basic_stringbuf<CharT> buf(STR("a very long string that exceeds the small string optimization buffer length"));
+ const CharT *p = buf.view().data();
+ std::basic_string<CharT> s = std::move(buf).str();
+ assert(s.data() == p); // the allocation was pilfered
+ assert(buf.view().empty());
+ }
+}
+
+struct StringBuf : std::stringbuf {
+ using basic_stringbuf::basic_stringbuf;
+ void public_setg(int a, int b, int c) {
+ char *p = eback();
+ this->setg(p + a, p + b, p + c);
+ }
+};
+
+static void test_altered_sequence_pointers() {
+ {
+ auto src = StringBuf("hello world", std::ios_base::in);
+ src.public_setg(4, 6, 9);
+ std::stringbuf dest;
+ dest = std::move(src);
+ std::string view = std::string(dest.view());
+ std::string str = std::move(dest).str();
+ assert(view == str);
+ LIBCPP_ASSERT(str == "o wor");
+ assert(dest.str().empty());
+ assert(dest.view().empty());
+ }
+ {
+ auto src = StringBuf("hello world", std::ios_base::in);
+ src.public_setg(4, 6, 9);
+ std::stringbuf dest;
+ dest.swap(src);
+ std::string view = std::string(dest.view());
+ std::string str = std::move(dest).str();
+ assert(view == str);
+ LIBCPP_ASSERT(str == "o wor");
+ assert(dest.str().empty());
+ assert(dest.view().empty());
+ }
}
int main(int, char**) {
@@ -44,5 +86,6 @@ int main(int, char**) {
#ifndef TEST_HAS_NO_WIDE_CHARACTERS
test<wchar_t>();
#endif
+ test_altered_sequence_pointers();
return 0;
}
diff --git a/libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.members/str.pass.cpp b/libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.members/str.pass.cpp
index 18a2337f6b7833f..43f7894f0d79636 100644
--- a/libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.members/str.pass.cpp
+++ b/libcxx/test/std/input.output/string.streams/stringbuf/stringbuf.members/str.pass.cpp
@@ -14,12 +14,46 @@
// void str(const basic_string<charT,traits,Allocator>& s);
#include <sstream>
+#include <string>
#include <cassert>
#include "test_macros.h"
+struct StringBuf : std::stringbuf {
+ explicit StringBuf(const char *s, std::ios_base::openmode mode) :
+ basic_stringbuf(s, mode) { }
+ void public_setg(int a, int b, int c) {
+ char *p = eback();
+ this->setg(p + a, p + b, p + c);
+ }
+};
+
+static void test_altered_sequence_pointers() {
+ {
+ StringBuf src("hello world", std::ios_base::in);
+ src.public_setg(4, 6, 9);
+ std::stringbuf dest;
+ dest = std::move(src);
+ std::string str = dest.str();
+ assert(5 <= str.size() && str.size() <= 11);
+ LIBCPP_ASSERT(str == "o wor");
+ LIBCPP_ASSERT(dest.str() == "o wor");
+ }
+ {
+ StringBuf src("hello world", std::ios_base::in);
+ src.public_setg(4, 6, 9);
+ std::stringbuf dest;
+ dest.swap(src);
+ std::string str = dest.str();
+ assert(5 <= str.size() && str.size() <= 11);
+ LIBCPP_ASSERT(str == "o wor");
+ LIBCPP_ASSERT(dest.str() == "o wor");
+ }
+}
+
int main(int, char**)
{
+ test_altered_sequence_pointers();
{
std::stringbuf buf("testing");
asser...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/67294
More information about the libcxx-commits
mailing list