[PATCH] Improve string::clear() performance
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.
2016-09-13 Cong Wang
PR libstdc++/77582
* libstdc++-v3/include/bits/basic_string.h: Change inline to a declaration.
* libstdc++-v3/include/bits/basic_string.tcc: Inline the
implementation based on _M_mutate, and use _S_empty_rep when possible.
* libstdc++-v3/testsuite/performance/21_strings/copy_and_clear.cc: New.
diff --git a/libstdc++-v3/include/bits/basic_string.h
b/libstdc++-v3/include/bits/basic_string.h
index e823f13..94d1df2 100644
--- a/libstdc++-v3/include/bits/basic_string.h
+++ b/libstdc++-v3/include/bits/basic_string.h
@@ -3686,8 +3686,7 @@ _GLIBCXX_END_NAMESPACE_CXX11
*/
// PR 56166: this should not throw.
void
- clear()
- { _M_mutate(0, this->size(), 0); }
+ clear();
/**
* Returns true if the %string is empty. Equivalent to
diff --git a/libstdc++-v3/include/bits/basic_string.tcc
b/libstdc++-v3/include/bits/basic_string.tcc
index 0080d2b..935932e 100644
--- a/libstdc++-v3/include/bits/basic_string.tcc
+++ b/libstdc++-v3/include/bits/basic_string.tcc
@@ -966,6 +966,28 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
template
void
basic_string<_CharT, _Traits, _Alloc>::
+clear()
+{
+ if (_M_rep()->_M_is_shared())
+{
+ const allocator_type __a = get_allocator();
+ _Rep* __r;
+
+#if _GLIBCXX_FULLY_DYNAMIC_STRING == 0
+ __r = &_S_empty_rep();
+#else
+ // Must reallocate.
+ __r = _Rep::_S_create(0, this->capacity(), __a);
+#endif
+ _M_rep()->_M_dispose(__a);
+ _M_data(__r->_M_refdata());
+}
+ _M_rep()->_M_set_length_and_sharable(0);
+}
+
+ template
+void
+basic_string<_CharT, _Traits, _Alloc>::
swap(basic_string& __s)
{
if (_M_rep()->_M_is_leaked())
diff --git a/libstdc++-v3/testsuite/performance/21_strings/copy_and_clear.cc
b/libstdc++-v3/testsuite/performance/21_strings/copy_and_clear.cc
new file mode 100644
index 000..708f053
--- /dev/null
+++ b/libstdc++-v3/testsuite/performance/21_strings/copy_and_clear.cc
@@ -0,0 +1,49 @@
+// Copyright (C) 2006-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/>.
+
+
+#include
+#include
+
+void benchmark(long len)
+{
+ using namespace std;
+ using namespace __gnu_test;
+
+ time_counter time;
+ resource_counter resource;
+
+ start_counters(time, resource);
+ for (long i = 0; i < len; ++i)
+{
+ string ss1("1");
+ string ss2(ss1);
+ ss1.clear();
+}
+ stop_counters(time, resource);
+
+ report_performance(__FILE__, "", time, resource);
+ clear_counters(time, resource);
+}
+
+int main()
+{
+ benchmark(100);
+ benchmark(1000);
+ benchmark(1);
+ return 0;
+}
Re: [PATCH] Improve string::clear() performance
On Wed, Sep 14, 2016 at 4:06 AM, Jonathan Wakely 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? 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. :) Thanks.
Re: [PATCH] Improve string::clear() performance
On Wed, Sep 14, 2016 at 10:28 AM, Jonathan Wakely wrote: > On 14/09/16 09:09 -0700, Cong Wang wrote: >> >> On Wed, Sep 14, 2016 at 4:06 AM, Jonathan Wakely >> wrote: >>> >>> 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(). > OK. My patch keeps the original logic except the case for _GLIBCXX_FULLY_DYNAMIC_STRING, should be at least as correct as the current code. ;) >> 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. Sure. For long term, I think gcc should have something as simple as 'Signed-off-by' for Linux kernel, otherwise too much work for first-time contributors like me. We all want to save time on this, don't we? ;) Thanks.
Re: [PATCH] Improve string::clear() performance
On Thu, Sep 15, 2016 at 2:08 AM, Jonathan Wakely wrote: > On 14/09/16 10:41 -0700, Cong Wang wrote: >> >> For long term, I think gcc should have something as simple as >> 'Signed-off-by' for Linux kernel, otherwise too much work for first-time >> contributors like me. We all want to save time on this, don't we? ;) > > > Signed-off-by wouldn't help. The copyright assignment is done so that > the FSF owns the copyright in the majority of the GCC code. If I add > a Signed-off-by to your patch that doesn't have any effect on your > copyright of the code. > > It certainly does add a hurdle for contributors to GCC, but it's not a > policy that is likely to change. > > Anyway, I'll fix this one way or another ASAP. Thanks again. Thanks for your kind explanation. I think I already finish the copyright thing after exchanging emails with ass...@gnu.org. So copyright is not a blocker for consideration of my patch any more.
Re: [PATCH] Improve string::clear() performance
On Fri, Sep 23, 2016 at 10:25 AM, Jonathan Wakely wrote: > > 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. Great! Thanks for letting me know.
