The move constructor for the SSO string uses assign(const basic_string&)
when either:
(1) the source string is "local" and so the contents of the small string
buffer need to be copied, or
(2) the allocator does not propagate and is_always_equal is false.
Case (1) is suboptimal, because the assign member is not noexcept and
the compiler isn't smart enough to see it won't actually throw in this
case. This causes extra code in the move assignment operator so that any
exception will be turned into a call to std::terminate. This can be
fixed by copying small strings inline instead of calling assign.
Case (2) is a bug, because the specific instances of the allocators
could be equal even if is_always_equal is false. This can result in an
unnecessary deep copy (and potentially-throwing allocation) when the
storage should be moved. This can be fixed by simply checking if the
allocators are equal.
PR libstdc++/87749
* include/bits/basic_string.h [_GLIBCXX_USE_CXX11_ABI]
(basic_string::operator=(basic_string&&)): For short strings copy the
buffer inline. Only fall back to using assign(const basic_string&) to
do a deep copy when reallocation is needed.
* testsuite/21_strings/basic_string/modifiers/assign/char/87749.cc:
New test.
* testsuite/21_strings/basic_string/modifiers/assign/char/
move_assign_optim.cc: New test.
* testsuite/21_strings/basic_string/modifiers/assign/wchar_t/87749.cc:
New test.
* testsuite/21_strings/basic_string/modifiers/assign/wchar_t/
move_assign_optim.cc: New test.
Tested powerpc64le-linux, committed to trunk.
I've also attached before.s and after.s showing the generated code for this
function on x86_64 with -std=gnu++17 -O3:
void test(std::string& target, std::string&& source) {
target = std::move(source);
}
That improvement comes from avoiding using assign(const basic_string&)
for copying small strings.
I plan to backport this to all branches (including gcc-6 if the RMs
approve it).
commit e15f57af2b2702552bb6c2fc5bdceb300ac008d2
Author: Jonathan Wakely <jwak...@redhat.com>
Date: Thu Oct 25 13:48:20 2018 +0100
PR libstdc++/87749 fix (and optimize) string move construction
The move constructor for the SSO string uses assign(const basic_string&)
when either:
(1) the source string is "local" and so the contents of the small string
buffer need to be copied, or
(2) the allocator does not propagate and is_always_equal is false.
Case (1) is suboptimal, because the assign member is not noexcept and
the compiler isn't smart enough to see it won't actually throw in this
case. This causes extra code in the move assignment operator so that any
exception will be turned into a call to std::terminate. This can be
fixed by copying small strings inline instead of calling assign.
Case (2) is a bug, because the specific instances of the allocators
could be equal even if is_always_equal is false. This can result in an
unnecessary deep copy (and potentially-throwing allocation) when the
storage should be moved. This can be fixed by simply checking if the
allocators are equal.
PR libstdc++/87749
* include/bits/basic_string.h [_GLIBCXX_USE_CXX11_ABI]
(basic_string::operator=(basic_string&&)): For short strings copy
the
buffer inline. Only fall back to using assign(const basic_string&)
to
do a deep copy when reallocation is needed.
* testsuite/21_strings/basic_string/modifiers/assign/char/87749.cc:
New test.
* testsuite/21_strings/basic_string/modifiers/assign/char/
move_assign_optim.cc: New test.
*
testsuite/21_strings/basic_string/modifiers/assign/wchar_t/87749.cc:
New test.
* testsuite/21_strings/basic_string/modifiers/assign/wchar_t/
move_assign_optim.cc: New test.
diff --git a/libstdc++-v3/include/bits/basic_string.h
b/libstdc++-v3/include/bits/basic_string.h
index ba94b51f616..ae6530fcdc9 100644
--- a/libstdc++-v3/include/bits/basic_string.h
+++ b/libstdc++-v3/include/bits/basic_string.h
@@ -744,20 +744,29 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
// Replace allocator if POCMA is true.
std::__alloc_on_move(_M_get_allocator(), __str._M_get_allocator());
- if (!__str._M_is_local()
- && (_Alloc_traits::_S_propagate_on_move_assign()
- || _Alloc_traits::_S_always_equal()))
+ if (__str._M_is_local())
{
+ // We've always got room for a short string, just copy it.
+ if (__str.size())
+ this->_S_copy(_M_data(), __str._M_data(), __str.size());
+ _M_set_length(__str.size());
+ }
+ else if (_Alloc_traits::_S_propagate_on_move_assign()
+ || _Alloc_traits::_S_always_equal()
+ || _M_get_allocator() == __str._M_get_allocator())
+ {
+ // Just move the allocated pointer, our allocator can free it.
pointer __data = nullptr;
size_type __capacity;
if (!_M_is_local())
{
if (_Alloc_traits::_S_always_equal())
{
+ // __str can reuse our existing storage.
__data = _M_data();
__capacity = _M_allocated_capacity;
}
- else
+ else // __str can't use it, so free it.
_M_destroy(_M_allocated_capacity);
}
@@ -772,8 +781,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
else
__str._M_data(__str._M_local_buf);
}
- else
- assign(__str);
+ else // Need to do a deep copy
+ assign(__str);
__str.clear();
return *this;
}
diff --git
a/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/assign/char/87749.cc
b/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/assign/char/87749.cc
new file mode 100644
index 00000000000..ef5f1e708ac
--- /dev/null
+++
b/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/assign/char/87749.cc
@@ -0,0 +1,78 @@
+// Copyright (C) 2018 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3. If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-do run { target c++11 } }
+
+// PR libstdc++/87749
+
+#include <string>
+#include <testsuite_hooks.h>
+
+bool oom = false;
+
+template<typename T>
+struct alloc
+{
+ using value_type = T;
+
+#if !_GLIBCXX_USE_CXX11_ABI
+ using size_type = unsigned long;
+ using difference_type = long;
+ using reference = T&;
+ using const_reference = T&;
+ using pointer = T*;
+ using const_pointer = const T*;
+ template<typename U>
+ struct rebind { using other = alloc<U>; };
+#endif
+
+ int not_empty = 0; // this makes is_always_equal false
+
+ alloc() = default;
+ template<typename U>
+ alloc(const alloc<U>&) { }
+
+ T* allocate(unsigned long n)
+ {
+ if (oom)
+ throw std::bad_alloc();
+ return std::allocator<T>().allocate(n);
+ }
+
+ void deallocate(T* p, unsigned long n)
+ {
+ std::allocator<T>().deallocate(p, n);
+ }
+};
+
+template<typename T, typename U>
+bool operator==(const alloc<T>&, const alloc<U>&) { return true; }
+
+template<typename T, typename U>
+bool operator!=(const alloc<T>&, const alloc<U>&) { return false; }
+
+int main()
+{
+ using string = std::basic_string<char, std::char_traits<char>, alloc<char>>;
+
+ string s = "PR libstdc++/87749 a string that is longer than a short string";
+ const auto ptr = s.c_str();
+ oom = true;
+ string ss;
+ ss = std::move(s); // allocators are equal, should not allocate new storage
+ VERIFY( ss.c_str() == ptr );
+}
diff --git
a/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/assign/char/move_assign_optim.cc
b/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/assign/char/move_assign_optim.cc
new file mode 100644
index 00000000000..b56bc50e1c1
--- /dev/null
+++
b/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/assign/char/move_assign_optim.cc
@@ -0,0 +1,35 @@
+// Copyright (C) 2018 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3. If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-options "-O1" }
+// { dg-do compile { target c++11 } }
+// { dg-final { scan-assembler-not "__throw_length_error" } }
+// { dg-final { scan-assembler-not "__throw_bad_alloc" } }
+
+#include <bits/c++config.h>
+#undef _GLIBCXX_EXTERN_TEMPLATE
+#include <string>
+
+void
+test01(std::string& target, std::string&& source)
+{
+ // The move assignment operator should be simple enough that the compiler
+ // can see that it never results in a length_error or bad_alloc exception
+ // (which would be turned into std::terminate by the noexcept on the
+ // assignment operator).
+ target = std::move(source);
+}
diff --git
a/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/assign/wchar_t/87749.cc
b/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/assign/wchar_t/87749.cc
new file mode 100644
index 00000000000..d4062a9e637
--- /dev/null
+++
b/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/assign/wchar_t/87749.cc
@@ -0,0 +1,79 @@
+// Copyright (C) 2018 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3. If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-do run { target c++11 } }
+
+// PR libstdc++/87749
+
+#include <string>
+#include <testsuite_hooks.h>
+
+bool oom = false;
+
+template<typename T>
+struct alloc
+{
+ using value_type = T;
+
+#if !_GLIBCXX_USE_CXX11_ABI
+ using size_type = unsigned long;
+ using difference_type = long;
+ using reference = T&;
+ using const_reference = T&;
+ using pointer = T*;
+ using const_pointer = const T*;
+ template<typename U>
+ struct rebind { using other = alloc<U>; };
+#endif
+
+ int not_empty = 0; // this makes is_always_equal false
+
+ alloc() = default;
+ template<typename U>
+ alloc(const alloc<U>&) { }
+
+ T* allocate(unsigned long n)
+ {
+ if (oom)
+ throw std::bad_alloc();
+ return std::allocator<T>().allocate(n);
+ }
+
+ void deallocate(T* p, unsigned long n)
+ {
+ std::allocator<T>().deallocate(p, n);
+ }
+};
+
+template<typename T, typename U>
+bool operator==(const alloc<T>&, const alloc<U>&) { return true; }
+
+template<typename T, typename U>
+bool operator!=(const alloc<T>&, const alloc<U>&) { return false; }
+
+int main()
+{
+ using string
+ = std::basic_string<wchar_t, std::char_traits<wchar_t>, alloc<wchar_t>>;
+
+ string s = L"PR libstdc++/87749 a string that is longer than a short string";
+ const auto ptr = s.c_str();
+ oom = true;
+ string ss;
+ ss = std::move(s); // allocators are equal, should not allocate new storage
+ VERIFY( ss.c_str() == ptr );
+}
diff --git
a/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/assign/wchar_t/move_assign_optim.cc
b/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/assign/wchar_t/move_assign_optim.cc
new file mode 100644
index 00000000000..f54ad36a5d0
--- /dev/null
+++
b/libstdc++-v3/testsuite/21_strings/basic_string/modifiers/assign/wchar_t/move_assign_optim.cc
@@ -0,0 +1,35 @@
+// Copyright (C) 2018 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library. This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3. If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-options "-O1" }
+// { dg-do compile { target c++11 } }
+// { dg-final { scan-assembler-not "__throw_length_error" } }
+// { dg-final { scan-assembler-not "__throw_bad_alloc" } }
+
+#include <bits/c++config.h>
+#undef _GLIBCXX_EXTERN_TEMPLATE
+#include <string>
+
+void
+test01(std::wstring& target, std::wstring&& source)
+{
+ // The move assignment operator should be simple enough that the compiler
+ // can see that it never results in a length_error or bad_alloc exception
+ // (which would be turned into std::terminate by the noexcept on the
+ // assignment operator).
+ target = std::move(source);
+}
.file "s.cc"
.text
.section .rodata.str1.1,"aMS",@progbits,1
.LC0:
.string "basic_string::_M_create"
.text
.p2align 4
.globl _Z4testRNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEOS4_
.type
_Z4testRNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEOS4_, @function
_Z4testRNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEOS4_:
.LFB1368:
.cfi_startproc
.cfi_personality 0x3,__gxx_personality_v0
.cfi_lsda 0x3,.LLSDA1368
pushq %r15
.cfi_def_cfa_offset 16
.cfi_offset 15, -16
leaq 16(%rsi), %rdx
pushq %r14
.cfi_def_cfa_offset 24
.cfi_offset 14, -24
pushq %r13
.cfi_def_cfa_offset 32
.cfi_offset 13, -32
leaq 16(%rdi), %r13
pushq %r12
.cfi_def_cfa_offset 40
.cfi_offset 12, -40
pushq %rbp
.cfi_def_cfa_offset 48
.cfi_offset 6, -48
movq %rdi, %rbp
pushq %rbx
.cfi_def_cfa_offset 56
.cfi_offset 3, -56
movq %rsi, %rbx
subq $8, %rsp
.cfi_def_cfa_offset 64
movq (%rsi), %rax
movq (%rdi), %r12
cmpq %rdx, %rax
je .L2
cmpq %r13, %r12
je .L26
movq %rax, (%rdi)
movq 8(%rsi), %rax
movq 16(%rdi), %rcx
movq %rax, 8(%rdi)
movq 16(%rsi), %rax
movq %rax, 16(%rdi)
testq %r12, %r12
je .L4
movq %r12, (%rsi)
movq %rcx, 16(%rsi)
.L5:
movq $0, 8(%rbx)
movb $0, (%r12)
addq $8, %rsp
.cfi_remember_state
.cfi_def_cfa_offset 56
popq %rbx
.cfi_def_cfa_offset 48
popq %rbp
.cfi_def_cfa_offset 40
popq %r12
.cfi_def_cfa_offset 32
popq %r13
.cfi_def_cfa_offset 24
popq %r14
.cfi_def_cfa_offset 16
popq %r15
.cfi_def_cfa_offset 8
ret
.p2align 4,,10
.p2align 3
.L26:
.cfi_restore_state
movq %rax, (%rdi)
movq 8(%rsi), %rax
movq %rax, 8(%rdi)
movq 16(%rsi), %rax
movq %rax, 16(%rdi)
.L4:
movq %rdx, (%rbx)
movq %rdx, %r12
jmp .L5
.p2align 4,,10
.p2align 3
.L2:
cmpq %rdi, %rsi
je .L14
movq 8(%rsi), %r14
cmpq %r13, %r12
je .L15
movq 16(%rdi), %rax
.L6:
cmpq %rax, %r14
ja .L27
.L7:
testq %r14, %r14
je .L10
movq (%rbx), %rsi
cmpq $1, %r14
je .L28
movq %r12, %rdi
movq %r14, %rdx
call memcpy
movq 0(%rbp), %r12
.L10:
movq %r14, 8(%rbp)
movb $0, (%r12,%r14)
movq (%rbx), %r12
jmp .L5
.p2align 4,,10
.p2align 3
.L14:
movq %rax, %r12
jmp .L5
.p2align 4,,10
.p2align 3
.L28:
movzbl (%rsi), %eax
movb %al, (%r12)
movq 0(%rbp), %r12
jmp .L10
.p2align 4,,10
.p2align 3
.L27:
movabsq $4611686018427387903, %rdx
cmpq %rdx, %r14
ja .L29
addq %rax, %rax
movq %r14, %r15
cmpq %rax, %r14
jnb .L9
cmpq %rdx, %rax
cmovbe %rax, %rdx
movq %rdx, %r15
.L9:
leaq 1(%r15), %rdi
call _Znwm
movq 0(%rbp), %rdi
movq %rax, %r12
cmpq %rdi, %r13
je .L13
call _ZdlPv
.L13:
movq %r12, 0(%rbp)
movq %r15, 16(%rbp)
jmp .L7
.p2align 4,,10
.p2align 3
.L15:
movl $15, %eax
jmp .L6
.L29:
movl $.LC0, %edi
call _ZSt20__throw_length_errorPKc
.cfi_endproc
.LFE1368:
.globl __gxx_personality_v0
.section .gcc_except_table,"a",@progbits
.LLSDA1368:
.byte 0xff
.byte 0xff
.byte 0x1
.uleb128 .LLSDACSE1368-.LLSDACSB1368
.LLSDACSB1368:
.LLSDACSE1368:
.text
.size
_Z4testRNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEOS4_,
.-_Z4testRNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEOS4_
.ident "GCC: (GNU) 9.0.0 20180824 (experimental) [master revision
18a4030f1ea:2845daafb41:509699701f824252abf953df5a3932171a708983]"
.section .note.GNU-stack,"",@progbits
.file "s.cc"
.text
.p2align 4
.globl _Z4testRNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEOS4_
.type
_Z4testRNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEOS4_, @function
_Z4testRNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEOS4_:
.LFB1368:
.cfi_startproc
pushq %rbp
.cfi_def_cfa_offset 16
.cfi_offset 6, -16
movq %rdi, %rbp
pushq %rbx
.cfi_def_cfa_offset 24
.cfi_offset 3, -24
movq %rsi, %rbx
leaq 16(%rbx), %rax
subq $8, %rsp
.cfi_def_cfa_offset 32
movq (%rsi), %rsi
movq (%rdi), %rdi
cmpq %rax, %rsi
je .L16
leaq 16(%rbp), %rdx
cmpq %rdx, %rdi
je .L17
movq %rsi, 0(%rbp)
movq 8(%rbx), %rcx
movq 16(%rbp), %rdx
movq %rcx, 8(%rbp)
movq 16(%rbx), %rcx
movq %rcx, 16(%rbp)
testq %rdi, %rdi
je .L7
movq %rdi, (%rbx)
movq %rdx, 16(%rbx)
.L5:
movq $0, 8(%rbx)
movb $0, (%rdi)
addq $8, %rsp
.cfi_remember_state
.cfi_def_cfa_offset 24
popq %rbx
.cfi_def_cfa_offset 16
popq %rbp
.cfi_def_cfa_offset 8
ret
.p2align 4,,10
.p2align 3
.L17:
.cfi_restore_state
movq %rsi, 0(%rbp)
movq 8(%rbx), %rdx
movq %rdx, 8(%rbp)
movq 16(%rbx), %rdx
movq %rdx, 16(%rbp)
.L7:
movq %rax, (%rbx)
movq %rax, %rdi
jmp .L5
.p2align 4,,10
.p2align 3
.L16:
movq 8(%rbx), %rdx
testq %rdx, %rdx
je .L3
cmpq $1, %rdx
je .L18
call memcpy
movq 8(%rbx), %rdx
movq 0(%rbp), %rdi
.L3:
movq %rdx, 8(%rbp)
movb $0, (%rdi,%rdx)
movq (%rbx), %rdi
jmp .L5
.p2align 4,,10
.p2align 3
.L18:
movzbl 16(%rbx), %eax
movb %al, (%rdi)
movq 8(%rbx), %rdx
movq 0(%rbp), %rdi
jmp .L3
.cfi_endproc
.LFE1368:
.size
_Z4testRNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEOS4_,
.-_Z4testRNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEOS4_
.ident "GCC: (GNU) 9.0.0 20180824 (experimental) [master revision
18a4030f1ea:2845daafb41:509699701f824252abf953df5a3932171a708983]"
.section .note.GNU-stack,"",@progbits
