-
Notifications
You must be signed in to change notification settings - Fork 12k
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
[libc++] Eliminate extra allocations from std::move(oss).str()
#67294
Conversation
@llvm/pr-subscribers-libcxx ChangesAdd 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 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:
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]
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
be43fa0
to
225f65c
Compare
d476f4e
to
2ee4903
Compare
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.
Looks mostly good. Just some nits regarding the tests.
...output/string.streams/istringstream/istringstream.members/str.allocator_propagation.pass.cpp
Outdated
Show resolved
Hide resolved
...output/string.streams/istringstream/istringstream.members/str.allocator_propagation.pass.cpp
Show resolved
Hide resolved
@philnik @ldionne
|
b5e81d3
to
42ec2de
Compare
b8fcd50
to
1b0dbb7
Compare
802c348
to
09a3f4c
Compare
@philnik777 can you land this for me? |
The CI didn't pass. Please update the PR and ping me again when the CI is green. |
CI has failed due to test failures. I've checked them locally on the |
Sorry, I missed that. It looks like the trunk CI is green, so the rebase should make sure everything is green. Feel free to ping me again when the CI is green. |
Add test coverage for the new behaviors, especially to verify that the returned string uses the correct allocator. Fixes llvm#64644
Thank you! It turned out that it was actually my fault. Now it's fixed, and CI is green. |
Add test coverage for the new behaviors, especially to verify that the returned string uses the correct allocator.
Fixes #64644
Migrated from https://reviews.llvm.org/D157776 — @philnik777 @pfusik
@ldionne @mordante
please take another look!