From: Jonathan Wakely <jwakely@redhat.com>
To: Will Hawkins <whh8b@obs.cr>
Cc: gcc-patches@gcc.gnu.org, libstdc++@gcc.gnu.org
Subject: Re: [PATCH] libstdc++: Refactor implementation of operator+ for std::string
Date: Tue, 8 Nov 2022 17:44:02 +0000 [thread overview]
Message-ID: <CACb0b4kyGbRo0fT-oiV8=o3c02NGLYHnHZA=BtpD1ZS8WWTayQ@mail.gmail.com> (raw)
In-Reply-To: <20221020000559.371886-1-whh8b@obs.cr>
On Thu, 20 Oct 2022 at 01:06, Will Hawkins wrote:
>
> Sorry for the delay. Tested on x86-64 Linux.
>
> -->8--
>
> After consultation with Jonathan, it seemed like a good idea to create a
> single function that performed one-allocation string concatenation that
> could be used by various different version of operator+. This patch adds
> such a function and calls it from the relevant implementations.
>
> libstdc++-v3/ChangeLog:
>
> * include/bits/basic_string.h:
> Add common function that performs single-allocation string
> concatenation. (__str_cat)
> Use __str_cat to perform optimized operator+, where relevant.
> * include/bits/basic_string.tcc::
> Remove single-allocation implementation of operator+.
>
> Signed-off-by: Will Hawkins <whh8b@obs.cr>
I've pushed this patch to trunk now. I changed the commit message
significantly though:
libstdc++: Refactor implementation of operator+ for std::string
Until now operator+(char*, string) and operator+(string, char*) had
different performance characteristics. The former required a single
memory allocation and the latter required two. This patch makes the
performance equal.
After consultation with Jonathan, it seemed like a good idea to create a
single function that performed one-allocation string concatenation that
could be used by various different version of operator+. This patch adds
such a function and calls it from the relevant implementations.
Co-authored-by: Jonathan Wakely <jwakely@redhat.com>
libstdc++-v3/ChangeLog:
* include/bits/basic_string.h (__str_cat): Add common function
that performs single-allocation string concatenation.
(operator+): Use __str_cat.
* include/bits/basic_string.tcc (operator+): Move to .h and
define inline using __str_cat.
Signed-off-by: Will Hawkins <whh8b@obs.cr>
Specifically, I restored part of your earlier commit's message, which
gives the necessary context for the commit. Just starting with "After
consultation with Jonathan, ..." doesn't say anything about the patch
itself and is not very helpful without the earlier context from the
mailing list.
I added myself as Co-author, since the new patch was largely based on
a patch I sent in a private email.
And I changed the changelog part to better meet the format of GNU ChangeLogs.
https://www.gnu.org/prep/standards/html_node/Style-of-Change-Logs.html
The change is on trunk now (and I didn't see any libgomp test failures
this time).
> ---
> libstdc++-v3/include/bits/basic_string.h | 66 ++++++++++++++++------
> libstdc++-v3/include/bits/basic_string.tcc | 41 --------------
> 2 files changed, 49 insertions(+), 58 deletions(-)
>
> diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h
> index cd244191df4..9c2b57f5a1d 100644
> --- a/libstdc++-v3/include/bits/basic_string.h
> +++ b/libstdc++-v3/include/bits/basic_string.h
> @@ -3485,6 +3485,24 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
> _GLIBCXX_END_NAMESPACE_CXX11
> #endif
>
> + template<typename _Str>
> + _GLIBCXX20_CONSTEXPR
> + inline _Str
> + __str_concat(typename _Str::value_type const* __lhs,
> + typename _Str::size_type __lhs_len,
> + typename _Str::value_type const* __rhs,
> + typename _Str::size_type __rhs_len,
> + typename _Str::allocator_type const& __a)
> + {
> + typedef typename _Str::allocator_type allocator_type;
> + typedef __gnu_cxx::__alloc_traits<allocator_type> _Alloc_traits;
> + _Str __str(_Alloc_traits::_S_select_on_copy(__a));
> + __str.reserve(__lhs_len + __rhs_len);
> + __str.append(__lhs, __lhs_len);
> + __str.append(__rhs, __rhs_len);
> + return __str;
> + }
> +
> // operator+
> /**
> * @brief Concatenate two strings.
> @@ -3494,13 +3512,14 @@ _GLIBCXX_END_NAMESPACE_CXX11
> */
> template<typename _CharT, typename _Traits, typename _Alloc>
> _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR
> - basic_string<_CharT, _Traits, _Alloc>
> + inline basic_string<_CharT, _Traits, _Alloc>
> operator+(const basic_string<_CharT, _Traits, _Alloc>& __lhs,
> const basic_string<_CharT, _Traits, _Alloc>& __rhs)
> {
> - basic_string<_CharT, _Traits, _Alloc> __str(__lhs);
> - __str.append(__rhs);
> - return __str;
> + typedef basic_string<_CharT, _Traits, _Alloc> _Str;
> + return std::__str_concat<_Str>(__lhs.c_str(), __lhs.size(),
> + __rhs.c_str(), __rhs.size(),
> + __lhs.get_allocator());
> }
>
> /**
> @@ -3511,9 +3530,16 @@ _GLIBCXX_END_NAMESPACE_CXX11
> */
> template<typename _CharT, typename _Traits, typename _Alloc>
> _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR
> - basic_string<_CharT,_Traits,_Alloc>
> + inline basic_string<_CharT,_Traits,_Alloc>
> operator+(const _CharT* __lhs,
> - const basic_string<_CharT,_Traits,_Alloc>& __rhs);
> + const basic_string<_CharT,_Traits,_Alloc>& __rhs)
> + {
> + __glibcxx_requires_string(__lhs);
> + typedef basic_string<_CharT, _Traits, _Alloc> _Str;
> + return std::__str_concat<_Str>(__lhs, _Traits::length(__lhs),
> + __rhs.c_str(), __rhs.size(),
> + __rhs.get_allocator());
> + }
>
> /**
> * @brief Concatenate character and string.
> @@ -3523,8 +3549,14 @@ _GLIBCXX_END_NAMESPACE_CXX11
> */
> template<typename _CharT, typename _Traits, typename _Alloc>
> _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR
> - basic_string<_CharT,_Traits,_Alloc>
> - operator+(_CharT __lhs, const basic_string<_CharT,_Traits,_Alloc>& __rhs);
> + inline basic_string<_CharT,_Traits,_Alloc>
> + operator+(_CharT __lhs, const basic_string<_CharT,_Traits,_Alloc>& __rhs)
> + {
> + typedef basic_string<_CharT, _Traits, _Alloc> _Str;
> + return std::__str_concat<_Str>(__builtin_addressof(__lhs), 1,
> + __rhs.c_str(), __rhs.size(),
> + __rhs.get_allocator());
> + }
>
> /**
> * @brief Concatenate string and C string.
> @@ -3538,11 +3570,12 @@ _GLIBCXX_END_NAMESPACE_CXX11
> operator+(const basic_string<_CharT, _Traits, _Alloc>& __lhs,
> const _CharT* __rhs)
> {
> - basic_string<_CharT, _Traits, _Alloc> __str(__lhs);
> - __str.append(__rhs);
> - return __str;
> + __glibcxx_requires_string(__rhs);
> + typedef basic_string<_CharT, _Traits, _Alloc> _Str;
> + return std::__str_concat<_Str>(__lhs.c_str(), __lhs.size(),
> + __rhs, _Traits::length(__rhs),
> + __lhs.get_allocator());
> }
> -
> /**
> * @brief Concatenate string and character.
> * @param __lhs First string.
> @@ -3554,11 +3587,10 @@ _GLIBCXX_END_NAMESPACE_CXX11
> inline basic_string<_CharT, _Traits, _Alloc>
> operator+(const basic_string<_CharT, _Traits, _Alloc>& __lhs, _CharT __rhs)
> {
> - typedef basic_string<_CharT, _Traits, _Alloc> __string_type;
> - typedef typename __string_type::size_type __size_type;
> - __string_type __str(__lhs);
> - __str.append(__size_type(1), __rhs);
> - return __str;
> + typedef basic_string<_CharT, _Traits, _Alloc> _Str;
> + return std::__str_concat<_Str>(__lhs.c_str(), __lhs.size(),
> + __builtin_addressof(__rhs), 1,
> + __lhs.get_allocator());
> }
>
> #if __cplusplus >= 201103L
> diff --git a/libstdc++-v3/include/bits/basic_string.tcc b/libstdc++-v3/include/bits/basic_string.tcc
> index 710c2dfe030..56114f120ba 100644
> --- a/libstdc++-v3/include/bits/basic_string.tcc
> +++ b/libstdc++-v3/include/bits/basic_string.tcc
> @@ -605,47 +605,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> #else
> # define _GLIBCXX_STRING_CONSTEXPR
> #endif
> -
> - template<typename _CharT, typename _Traits, typename _Alloc>
> - _GLIBCXX20_CONSTEXPR
> - basic_string<_CharT, _Traits, _Alloc>
> - operator+(const _CharT* __lhs,
> - const basic_string<_CharT, _Traits, _Alloc>& __rhs)
> - {
> - __glibcxx_requires_string(__lhs);
> - typedef basic_string<_CharT, _Traits, _Alloc> __string_type;
> - typedef typename __string_type::size_type __size_type;
> - typedef typename __gnu_cxx::__alloc_traits<_Alloc>::template
> - rebind<_CharT>::other _Char_alloc_type;
> - typedef __gnu_cxx::__alloc_traits<_Char_alloc_type> _Alloc_traits;
> - const __size_type __len = _Traits::length(__lhs);
> - __string_type __str(_Alloc_traits::_S_select_on_copy(
> - __rhs.get_allocator()));
> - __str.reserve(__len + __rhs.size());
> - __str.append(__lhs, __len);
> - __str.append(__rhs);
> - return __str;
> - }
> -
> - template<typename _CharT, typename _Traits, typename _Alloc>
> - _GLIBCXX20_CONSTEXPR
> - basic_string<_CharT, _Traits, _Alloc>
> - operator+(_CharT __lhs, const basic_string<_CharT, _Traits, _Alloc>& __rhs)
> - {
> - typedef basic_string<_CharT, _Traits, _Alloc> __string_type;
> - typedef typename __string_type::size_type __size_type;
> - typedef typename __gnu_cxx::__alloc_traits<_Alloc>::template
> - rebind<_CharT>::other _Char_alloc_type;
> - typedef __gnu_cxx::__alloc_traits<_Char_alloc_type> _Alloc_traits;
> - __string_type __str(_Alloc_traits::_S_select_on_copy(
> - __rhs.get_allocator()));
> - const __size_type __len = __rhs.size();
> - __str.reserve(__len + 1);
> - __str.append(__size_type(1), __lhs);
> - __str.append(__rhs);
> - return __str;
> - }
> -
> template<typename _CharT, typename _Traits, typename _Alloc>
> _GLIBCXX_STRING_CONSTEXPR
> typename basic_string<_CharT, _Traits, _Alloc>::size_type
> --
> 2.37.3
>
next prev parent reply other threads:[~2022-11-08 17:44 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-20 0:05 Will Hawkins
2022-11-02 20:25 ` Will Hawkins
2022-11-08 17:44 ` Jonathan Wakely [this message]
-- strict thread matches above, loose matches on Subject: below --
2022-09-05 18:30 Will Hawkins
2022-09-08 17:50 ` François Dumont
2022-09-08 18:05 ` Jonathan Wakely
2022-09-09 13:53 ` Will Hawkins
2022-09-09 20:28 ` Jonathan Wakely
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CACb0b4kyGbRo0fT-oiV8=o3c02NGLYHnHZA=BtpD1ZS8WWTayQ@mail.gmail.com' \
--to=jwakely@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=libstdc++@gcc.gnu.org \
--cc=whh8b@obs.cr \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).