This is the mail archive of the
libstdc++@gcc.gnu.org
mailing list for the libstdc++ project.
Re: [PATCH] Improve string::clear() performance
On 14/09/16 18:28 +0100, Jonathan Wakely wrote:
On 14/09/16 09:09 -0700, Cong Wang wrote:
On Wed, Sep 14, 2016 at 4:06 AM, Jonathan Wakely <jwakely@redhat.com> wrote:
On 13/09/16 11:02 -0700, Cong Wang wrote:
In !_GLIBCXX_USE_CXX11_ABI implementation, string::clear() calls
_M_mutate(), which could allocate memory as we do COW. This hurts
performance when string::clear() is on the hot path.
This patch improves it by using _S_empty_rep directly when
_GLIBCXX_FULLY_DYNAMIC_STRING is not enabled. And Linux distro like
Fedora doesn't enable this, this is why we caught it.
The copy-and-clear test shows it improves by 50%.
Ran all testsuites on Linux-x64.
Thank you for the patch (and changelog and test results!).
Do you have a GCC copyright assignment in place? See
https://gcc.gnu.org/contribute.html#legal for details.
Oh, didn't notice this, I thought gcc has something as quick
as the 'Signed-off-by' for Linux kernel (I am a Linux kernel developer).
I will do it.
If I understand the purpose of the change correctly, it's similar to
https://gcc.gnu.org/ml/gcc-patches/2014-06/msg00278.html - is that
right?
Oh, yes, I didn't know your patch because I don't subscribe
gcc mailing list. I am wondering why your patch is not merged
after 2+ years?
As I said in the patch email, I'm not sure if it's conforming, due to
clearing the string's cpacity() as well as its size().
Please let me know what you prefer: 1) You update your patch
and get it merged; 2) Use my patch if it looks good. I am fine with
either way. :)
I'll refresh my memory of the various issues and reconsider applying
my patch (that way we don't need to wait for your copyright
assignment). The performance benefits you measured make it more
compelling.
I'm applying this patch, which is my one from 2014 with a check for
whether the string is shared, so we just set the length to zero (and
don't throw away reusable capacity) when it's not shared.
Tested x86_64-linux, committed to trunk.
commit b9546ed53b169ffb809371231342eeb63595d8bc
Author: Jonathan Wakely <jwakely@redhat.com>
Date: Fri Sep 23 18:01:25 2016 +0100
Avoid reallocation for basic_string::clear()
PR libstdc++/56166
PR libstdc++/77582
* include/bits/basic_string.h (basic_string::clear()): Drop reference
and use empty rep.
* include/ext/rc_string_base.h (__rc_string_base::_M_clear()):
Likewise.
* testsuite/21_strings/basic_string/56166.cc: New.
* testsuite/ext/vstring/modifiers/clear/56166.cc: New.
diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h
index 2708cbc..7a4204e 100644
--- a/libstdc++-v3/include/bits/basic_string.h
+++ b/libstdc++-v3/include/bits/basic_string.h
@@ -3690,10 +3690,24 @@ _GLIBCXX_END_NAMESPACE_CXX11
/**
* Erases the string, making it empty.
*/
+#if _GLIBCXX_FULLY_DYNAMIC_STRING == 0
+ void
+ clear() _GLIBCXX_NOEXCEPT
+ {
+ if (_M_rep()->_M_is_shared())
+ {
+ _M_rep()->_M_dispose(this->get_allocator());
+ _M_data(_S_empty_rep()._M_refdata());
+ }
+ else
+ _M_rep()->_M_set_length_and_sharable(0);
+ }
+#else
// PR 56166: this should not throw.
void
clear()
{ _M_mutate(0, this->size(), 0); }
+#endif
/**
* Returns true if the %string is empty. Equivalent to
diff --git a/libstdc++-v3/include/ext/rc_string_base.h b/libstdc++-v3/include/ext/rc_string_base.h
index 1c1ed87..eab3461 100644
--- a/libstdc++-v3/include/ext/rc_string_base.h
+++ b/libstdc++-v3/include/ext/rc_string_base.h
@@ -354,7 +354,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
void
_M_clear()
- { _M_erase(size_type(0), _M_length()); }
+ {
+ _M_dispose();
+ _M_data(_S_empty_rep._M_refcopy());
+ }
bool
_M_compare(const __rc_string_base&) const
diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/56166.cc b/libstdc++-v3/testsuite/21_strings/basic_string/56166.cc
new file mode 100644
index 0000000..3d4d876
--- /dev/null
+++ b/libstdc++-v3/testsuite/21_strings/basic_string/56166.cc
@@ -0,0 +1,93 @@
+// Copyright (C) 2016 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 } }
+
+// libstdc++/56166
+
+#ifndef _GLIBCXX_USE_CXX11_ABI
+# define _GLIBCXX_USE_CXX11_ABI 0
+#endif
+#include <string>
+#include <new>
+
+static int fail_after = -1;
+
+template<typename T>
+ struct Allocator
+ {
+ using value_type = T;
+
+ // Need these typedefs because COW string doesn't use allocator_traits.
+ using pointer = T*;
+ using const_pointer = const T*;
+ using reference = T&;
+ using const_reference = const T&;
+ using difference_type = long;
+ using size_type = unsigned long;
+ template<typename U>
+ struct rebind {
+ using other = Allocator<U>;
+ };
+
+ Allocator() { }
+
+ template<typename U>
+ Allocator(const Allocator<U>&) { }
+
+ T* allocate(size_type n)
+ {
+ if (fail_after >= 0) {
+ if (fail_after-- == 0) {
+ throw std::bad_alloc();
+ }
+ }
+ return (T*)new char[n * sizeof(T)];
+ }
+
+ void deallocate(T* p, size_type)
+ {
+ delete[] (char*)p;
+ }
+ };
+
+template<typename T, typename U>
+ bool operator==(const Allocator<T>&, const Allocator<U>&) { return true; }
+template<typename T, typename U>
+ bool operator!=(const Allocator<T>&, const Allocator<U>&) { return false; }
+
+using string = std::basic_string<char, std::char_traits<char>, Allocator<char>>;
+
+string f()
+{
+ string s1("xxxxxx");
+ string s2 = s1;
+ s1.clear();
+ return s2;
+}
+
+int main()
+{
+ for (int i = 0; i < 10; i++) {
+ try {
+ fail_after = i;
+ f();
+ break;
+ } catch (std::bad_alloc) {
+ }
+ }
+}
diff --git a/libstdc++-v3/testsuite/ext/vstring/modifiers/clear/56166.cc b/libstdc++-v3/testsuite/ext/vstring/modifiers/clear/56166.cc
new file mode 100644
index 0000000..109e622
--- /dev/null
+++ b/libstdc++-v3/testsuite/ext/vstring/modifiers/clear/56166.cc
@@ -0,0 +1,96 @@
+// Copyright (C) 2016 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 } }
+
+// libstdc++/56166
+
+#ifndef _GLIBCXX_USE_CXX11_ABI
+# define _GLIBCXX_USE_CXX11_ABI 0
+#endif
+#include <ext/vstring.h>
+#include <new>
+
+static int fail_after = -1;
+
+template<typename T>
+ struct Allocator
+ {
+ using value_type = T;
+
+ // Need these typedefs because COW string doesn't use allocator_traits.
+ using pointer = T*;
+ using const_pointer = const T*;
+ using reference = T&;
+ using const_reference = const T&;
+ using difference_type = long;
+ using size_type = unsigned long;
+ template<typename U>
+ struct rebind {
+ using other = Allocator<U>;
+ };
+
+ Allocator() { }
+
+ template<typename U>
+ Allocator(const Allocator<U>&) { }
+
+ T* allocate(size_type n)
+ {
+ if (fail_after >= 0) {
+ if (fail_after-- == 0) {
+ throw std::bad_alloc();
+ }
+ }
+ return (T*)new char[n * sizeof(T)];
+ }
+
+ void deallocate(T* p, size_type)
+ {
+ delete[] (char*)p;
+ }
+ };
+
+template<typename T, typename U>
+ bool operator==(const Allocator<T>&, const Allocator<U>&) { return true; }
+template<typename T, typename U>
+ bool operator!=(const Allocator<T>&, const Allocator<U>&) { return false; }
+
+
+using string = __gnu_cxx::__versa_string<char, std::char_traits<char>,
+ Allocator<char>,
+ __gnu_cxx::__rc_string_base>;
+
+string f()
+{
+ string s1("xxxxxx");
+ string s2 = s1;
+ s1.clear();
+ return s2;
+}
+
+int main()
+{
+ for (int i = 0; i < 10; i++) {
+ try {
+ fail_after = i;
+ f();
+ break;
+ } catch (std::bad_alloc) {
+ }
+ }
+}