* [PATCH] libstdc++: Optimize operator+(string/char*, string/char*) equally @ 2022-08-22 18:15 whh8b 2022-08-22 18:15 ` whh8b 0 siblings, 1 reply; 13+ messages in thread From: whh8b @ 2022-08-22 18:15 UTC (permalink / raw) To: libstdc++, gcc-patches After consultation with Jonathan, we realized that there was a missed optimization opportunity in the implementation of the various forms of operator+ for string. operator+(char *, string) required a single allocation but operator+(string, char*) required two. This patch attempts to change that asymmetry. Again, I have been a longtime reader of the source code but this patch is one of my first attempts at contributing. I have attempted to follow all the rules but I am sure that I missed something. Please let me know if/how I can change the patch to make it acceptable. Thanks for all your hard work maintaing this vital piece of software! Will ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] libstdc++: Optimize operator+(string/char*, string/char*) equally 2022-08-22 18:15 [PATCH] libstdc++: Optimize operator+(string/char*, string/char*) equally whh8b @ 2022-08-22 18:15 ` whh8b 2022-08-23 16:33 ` Jonathan Wakely 2022-08-24 6:16 ` [PATCH v2] libstdc++: Optimize operator+(string/char*,string/char*) whh8b 0 siblings, 2 replies; 13+ messages in thread From: whh8b @ 2022-08-22 18:15 UTC (permalink / raw) To: libstdc++, gcc-patches; +Cc: Will Hawkins From: Will Hawkins <whh8b@obs.cr> 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. libstdc++-v3/ChangeLog: * libstdc++-v3/include/bits/basic_string.h (operator+(string, char*)): Remove naive implementation. * libstdc++-v3/include/bits/basic_string.tcc (operator+(string, char*)): Add single-allocation implementation. --- libstdc++-v3/include/bits/basic_string.h | 7 +------ libstdc++-v3/include/bits/basic_string.tcc | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h index b04fba95678..bc048fc0689 100644 --- a/libstdc++-v3/include/bits/basic_string.h +++ b/libstdc++-v3/include/bits/basic_string.h @@ -3523,12 +3523,7 @@ _GLIBCXX_END_NAMESPACE_CXX11 _GLIBCXX20_CONSTEXPR inline basic_string<_CharT, _Traits, _Alloc> operator+(const basic_string<_CharT, _Traits, _Alloc>& __lhs, - const _CharT* __rhs) - { - basic_string<_CharT, _Traits, _Alloc> __str(__lhs); - __str.append(__rhs); - return __str; - } + const _CharT* __rhs); /** * @brief Concatenate string and character. diff --git a/libstdc++-v3/include/bits/basic_string.tcc b/libstdc++-v3/include/bits/basic_string.tcc index 4563c61429a..95ba8e503e9 100644 --- a/libstdc++-v3/include/bits/basic_string.tcc +++ b/libstdc++-v3/include/bits/basic_string.tcc @@ -640,6 +640,27 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION return __str; } + template<typename _CharT, typename _Traits, typename _Alloc> + _GLIBCXX20_CONSTEXPR + basic_string<_CharT, _Traits, _Alloc> + operator+(const basic_string<_CharT, _Traits, _Alloc>& __lhs, + const _CharT* __rhs) + { + __glibcxx_requires_string(__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; + const __size_type __len = _Traits::length(__rhs); + __string_type __str(_Alloc_traits::_S_select_on_copy( + __lhs.get_allocator())); + __str.reserve(__len + __lhs.size()); + __str.append(__lhs); + __str.append(__rhs, __len); + return __str; + } + template<typename _CharT, typename _Traits, typename _Alloc> _GLIBCXX_STRING_CONSTEXPR typename basic_string<_CharT, _Traits, _Alloc>::size_type -- 2.34.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] libstdc++: Optimize operator+(string/char*, string/char*) equally 2022-08-22 18:15 ` whh8b @ 2022-08-23 16:33 ` Jonathan Wakely 2022-08-24 6:18 ` Will Hawkins 2022-08-24 6:16 ` [PATCH v2] libstdc++: Optimize operator+(string/char*,string/char*) whh8b 1 sibling, 1 reply; 13+ messages in thread From: Jonathan Wakely @ 2022-08-23 16:33 UTC (permalink / raw) To: whh8b; +Cc: libstdc++, gcc-patches On Mon, 22 Aug 2022 at 19:15, Will Hawkins wrote: > > 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. If you don't have a GCC copyright assignment on file with the FSF then please follow the procedure described at https://gcc.gnu.org/dco.html > libstdc++-v3/ChangeLog: > * libstdc++-v3/include/bits/basic_string.h (operator+(string, char*)): > Remove naive implementation. > * libstdc++-v3/include/bits/basic_string.tcc (operator+(string, char*)): > Add single-allocation implementation. > --- > libstdc++-v3/include/bits/basic_string.h | 7 +------ > libstdc++-v3/include/bits/basic_string.tcc | 21 +++++++++++++++++++++ > 2 files changed, 22 insertions(+), 6 deletions(-) > > diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h > index b04fba95678..bc048fc0689 100644 > --- a/libstdc++-v3/include/bits/basic_string.h > +++ b/libstdc++-v3/include/bits/basic_string.h > @@ -3523,12 +3523,7 @@ _GLIBCXX_END_NAMESPACE_CXX11 > _GLIBCXX20_CONSTEXPR > inline basic_string<_CharT, _Traits, _Alloc> Please remove the 'inline' specifier here, since you're moving the definition into the non-inline .tcc file. There's a separate discussion to be had about whether these operator+ overloads *should* be inline. But for the purposes of this change, we want these two operator+ overloads to be consistent, and so they should both be non-inline. > operator+(const basic_string<_CharT, _Traits, _Alloc>& __lhs, > - const _CharT* __rhs) > - { > - basic_string<_CharT, _Traits, _Alloc> __str(__lhs); > - __str.append(__rhs); > - return __str; > - } > + const _CharT* __rhs); > > /** > * @brief Concatenate string and character. > diff --git a/libstdc++-v3/include/bits/basic_string.tcc b/libstdc++-v3/include/bits/basic_string.tcc > index 4563c61429a..95ba8e503e9 100644 > --- a/libstdc++-v3/include/bits/basic_string.tcc > +++ b/libstdc++-v3/include/bits/basic_string.tcc > @@ -640,6 +640,27 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > return __str; > } > > + template<typename _CharT, typename _Traits, typename _Alloc> > + _GLIBCXX20_CONSTEXPR > + basic_string<_CharT, _Traits, _Alloc> > + operator+(const basic_string<_CharT, _Traits, _Alloc>& __lhs, > + const _CharT* __rhs) > + { > + __glibcxx_requires_string(__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; > + const __size_type __len = _Traits::length(__rhs); > + __string_type __str(_Alloc_traits::_S_select_on_copy( > + __lhs.get_allocator())); > + __str.reserve(__len + __lhs.size()); > + __str.append(__lhs); > + __str.append(__rhs, __len); > + return __str; > + } > + > template<typename _CharT, typename _Traits, typename _Alloc> > _GLIBCXX_STRING_CONSTEXPR > typename basic_string<_CharT, _Traits, _Alloc>::size_type > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] libstdc++: Optimize operator+(string/char*, string/char*) equally 2022-08-23 16:33 ` Jonathan Wakely @ 2022-08-24 6:18 ` Will Hawkins 2022-08-24 9:49 ` Jonathan Wakely 0 siblings, 1 reply; 13+ messages in thread From: Will Hawkins @ 2022-08-24 6:18 UTC (permalink / raw) To: Jonathan Wakely; +Cc: libstdc++, gcc-patches On Tue, Aug 23, 2022 at 12:33 PM Jonathan Wakely <jwakely@redhat.com> wrote: > > On Mon, 22 Aug 2022 at 19:15, Will Hawkins wrote: > > > > 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. > > If you don't have a GCC copyright assignment on file with the FSF then > please follow the procedure described at https://gcc.gnu.org/dco.html Thank you. > > > > libstdc++-v3/ChangeLog: > > * libstdc++-v3/include/bits/basic_string.h (operator+(string, char*)): > > Remove naive implementation. > > * libstdc++-v3/include/bits/basic_string.tcc (operator+(string, char*)): > > Add single-allocation implementation. > > --- > > libstdc++-v3/include/bits/basic_string.h | 7 +------ > > libstdc++-v3/include/bits/basic_string.tcc | 21 +++++++++++++++++++++ > > 2 files changed, 22 insertions(+), 6 deletions(-) > > > > diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h > > index b04fba95678..bc048fc0689 100644 > > --- a/libstdc++-v3/include/bits/basic_string.h > > +++ b/libstdc++-v3/include/bits/basic_string.h > > @@ -3523,12 +3523,7 @@ _GLIBCXX_END_NAMESPACE_CXX11 > > _GLIBCXX20_CONSTEXPR > > inline basic_string<_CharT, _Traits, _Alloc> > > Please remove the 'inline' specifier here, since you're moving the > definition into the non-inline .tcc file. > > There's a separate discussion to be had about whether these operator+ > overloads *should* be inline. But for the purposes of this change, we > want these two operator+ overloads to be consistent, and so they > should both be non-inline. Thank you for the feedback. I sent out a v2 of the patch. Again, I hope that I followed the proper procedure by having my mailer put the patch in reply to my previous message. Thank you again! Will > > > operator+(const basic_string<_CharT, _Traits, _Alloc>& __lhs, > > - const _CharT* __rhs) > > - { > > - basic_string<_CharT, _Traits, _Alloc> __str(__lhs); > > - __str.append(__rhs); > > - return __str; > > - } > > + const _CharT* __rhs); > > > > /** > > * @brief Concatenate string and character. > > diff --git a/libstdc++-v3/include/bits/basic_string.tcc b/libstdc++-v3/include/bits/basic_string.tcc > > index 4563c61429a..95ba8e503e9 100644 > > --- a/libstdc++-v3/include/bits/basic_string.tcc > > +++ b/libstdc++-v3/include/bits/basic_string.tcc > > @@ -640,6 +640,27 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > return __str; > > } > > > > + template<typename _CharT, typename _Traits, typename _Alloc> > > + _GLIBCXX20_CONSTEXPR > > + basic_string<_CharT, _Traits, _Alloc> > > + operator+(const basic_string<_CharT, _Traits, _Alloc>& __lhs, > > + const _CharT* __rhs) > > + { > > + __glibcxx_requires_string(__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; > > + const __size_type __len = _Traits::length(__rhs); > > + __string_type __str(_Alloc_traits::_S_select_on_copy( > > + __lhs.get_allocator())); > > + __str.reserve(__len + __lhs.size()); > > + __str.append(__lhs); > > + __str.append(__rhs, __len); > > + return __str; > > + } > > + > > template<typename _CharT, typename _Traits, typename _Alloc> > > _GLIBCXX_STRING_CONSTEXPR > > typename basic_string<_CharT, _Traits, _Alloc>::size_type > > -- > > 2.34.1 > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] libstdc++: Optimize operator+(string/char*, string/char*) equally 2022-08-24 6:18 ` Will Hawkins @ 2022-08-24 9:49 ` Jonathan Wakely 0 siblings, 0 replies; 13+ messages in thread From: Jonathan Wakely @ 2022-08-24 9:49 UTC (permalink / raw) To: Will Hawkins; +Cc: libstdc++, gcc-patches On Wed, 24 Aug 2022 at 07:18, Will Hawkins <hawkinsw@obs.cr> wrote: > > On Tue, Aug 23, 2022 at 12:33 PM Jonathan Wakely <jwakely@redhat.com> wrote: > > > > On Mon, 22 Aug 2022 at 19:15, Will Hawkins wrote: > > > > > > 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. > > > > If you don't have a GCC copyright assignment on file with the FSF then > > please follow the procedure described at https://gcc.gnu.org/dco.html > > Thank you. > > > > > > > > libstdc++-v3/ChangeLog: > > > * libstdc++-v3/include/bits/basic_string.h (operator+(string, char*)): > > > Remove naive implementation. > > > * libstdc++-v3/include/bits/basic_string.tcc (operator+(string, char*)): > > > Add single-allocation implementation. > > > --- > > > libstdc++-v3/include/bits/basic_string.h | 7 +------ > > > libstdc++-v3/include/bits/basic_string.tcc | 21 +++++++++++++++++++++ > > > 2 files changed, 22 insertions(+), 6 deletions(-) > > > > > > diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h > > > index b04fba95678..bc048fc0689 100644 > > > --- a/libstdc++-v3/include/bits/basic_string.h > > > +++ b/libstdc++-v3/include/bits/basic_string.h > > > @@ -3523,12 +3523,7 @@ _GLIBCXX_END_NAMESPACE_CXX11 > > > _GLIBCXX20_CONSTEXPR > > > inline basic_string<_CharT, _Traits, _Alloc> > > > > Please remove the 'inline' specifier here, since you're moving the > > definition into the non-inline .tcc file. > > > > There's a separate discussion to be had about whether these operator+ > > overloads *should* be inline. But for the purposes of this change, we > > want these two operator+ overloads to be consistent, and so they > > should both be non-inline. > > Thank you for the feedback. I sent out a v2 of the patch. Again, I > hope that I followed the proper procedure by having my mailer put the > patch in reply to my previous message. It looks like the patch got attached in this thread, not the [PATCH v2] thread: https://gcc.gnu.org/pipermail/gcc-patches/2022-August/600176.html Presumably it was meant as a reply to: https://gcc.gnu.org/pipermail/gcc-patches/2022-August/600175.html It's more conventional to put the patch in the same email, not as a separate reply, which would avoid that problem. You can use git scissors to separate the patch submission from the preceding discussion and comments, see https://git-scm.com/docs/git-mailinfo#Documentation/git-mailinfo.txt---scissors For example, see my patches like the one at https://gcc.gnu.org/pipermail/gcc-patches/2022-August/600109.html where the "informational" part comes first, describing where it was tested, then the patch (and its commit msg) come after the -- >8 -- scissors. If you use git send-email to send mails, you can use --in-reply-to=$MessageId to make the email a reply to the specified $MessageId from a previous mail in the thread. Anyway, the v2 patch looks fine and I'll apply it to trunk after testing - thanks! ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] libstdc++: Optimize operator+(string/char*,string/char*) 2022-08-22 18:15 ` whh8b 2022-08-23 16:33 ` Jonathan Wakely @ 2022-08-24 6:16 ` whh8b 2022-08-24 6:16 ` [PATCH] libstdc++: Optimize operator+(string/char*,string/char*) equally whh8b 1 sibling, 1 reply; 13+ messages in thread From: whh8b @ 2022-08-24 6:16 UTC (permalink / raw) To: libstdc++, gcc-patches A revision of the original patch -- based on the feedback from Jonathan -- that removes the `inline` specifier is attached. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] libstdc++: Optimize operator+(string/char*,string/char*) equally 2022-08-24 6:16 ` [PATCH v2] libstdc++: Optimize operator+(string/char*,string/char*) whh8b @ 2022-08-24 6:16 ` whh8b 2022-08-24 6:16 ` [PATCH] libstdc++: Optimize operator+(string/char*, string/char*) equally whh8b 2022-08-24 14:24 ` Jonathan Wakely 0 siblings, 2 replies; 13+ messages in thread From: whh8b @ 2022-08-24 6:16 UTC (permalink / raw) To: libstdc++, gcc-patches; +Cc: Will Hawkins From: Will Hawkins <whh8b@obs.cr> 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. libstdc++-v3/ChangeLog: * libstdc++-v3/include/bits/basic_string.h (operator+(string, char*)): Remove naive implementation. * libstdc++-v3/include/bits/basic_string.tcc (operator+(string, char*)): Add single-allocation implementation. Signed-off-by: Will Hawkins <whh8b@obs.cr> --- libstdc++-v3/include/bits/basic_string.h | 9 ++------- libstdc++-v3/include/bits/basic_string.tcc | 21 +++++++++++++++++++++ 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h index b04fba95678..fa6738925bb 100644 --- a/libstdc++-v3/include/bits/basic_string.h +++ b/libstdc++-v3/include/bits/basic_string.h @@ -3521,14 +3521,9 @@ _GLIBCXX_END_NAMESPACE_CXX11 */ template<typename _CharT, typename _Traits, typename _Alloc> _GLIBCXX20_CONSTEXPR - inline basic_string<_CharT, _Traits, _Alloc> + basic_string<_CharT, _Traits, _Alloc> operator+(const basic_string<_CharT, _Traits, _Alloc>& __lhs, - const _CharT* __rhs) - { - basic_string<_CharT, _Traits, _Alloc> __str(__lhs); - __str.append(__rhs); - return __str; - } + const _CharT* __rhs); /** * @brief Concatenate string and character. diff --git a/libstdc++-v3/include/bits/basic_string.tcc b/libstdc++-v3/include/bits/basic_string.tcc index 4563c61429a..95ba8e503e9 100644 --- a/libstdc++-v3/include/bits/basic_string.tcc +++ b/libstdc++-v3/include/bits/basic_string.tcc @@ -640,6 +640,27 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION return __str; } + template<typename _CharT, typename _Traits, typename _Alloc> + _GLIBCXX20_CONSTEXPR + basic_string<_CharT, _Traits, _Alloc> + operator+(const basic_string<_CharT, _Traits, _Alloc>& __lhs, + const _CharT* __rhs) + { + __glibcxx_requires_string(__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; + const __size_type __len = _Traits::length(__rhs); + __string_type __str(_Alloc_traits::_S_select_on_copy( + __lhs.get_allocator())); + __str.reserve(__len + __lhs.size()); + __str.append(__lhs); + __str.append(__rhs, __len); + return __str; + } + template<typename _CharT, typename _Traits, typename _Alloc> _GLIBCXX_STRING_CONSTEXPR typename basic_string<_CharT, _Traits, _Alloc>::size_type -- 2.34.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] libstdc++: Optimize operator+(string/char*, string/char*) equally 2022-08-24 6:16 ` [PATCH] libstdc++: Optimize operator+(string/char*,string/char*) equally whh8b @ 2022-08-24 6:16 ` whh8b 2022-08-24 14:24 ` Jonathan Wakely 1 sibling, 0 replies; 13+ messages in thread From: whh8b @ 2022-08-24 6:16 UTC (permalink / raw) To: libstdc++, gcc-patches; +Cc: Will Hawkins From: Will Hawkins <whh8b@obs.cr> 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. libstdc++-v3/ChangeLog: * libstdc++-v3/include/bits/basic_string.h (operator+(string, char*)): Remove naive implementation. * libstdc++-v3/include/bits/basic_string.tcc (operator+(string, char*)): Add single-allocation implementation. Signed-off-by: Will Hawkins <whh8b@obs.cr> --- libstdc++-v3/include/bits/basic_string.h | 9 ++------- libstdc++-v3/include/bits/basic_string.tcc | 21 +++++++++++++++++++++ 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h index b04fba95678..fa6738925bb 100644 --- a/libstdc++-v3/include/bits/basic_string.h +++ b/libstdc++-v3/include/bits/basic_string.h @@ -3521,14 +3521,9 @@ _GLIBCXX_END_NAMESPACE_CXX11 */ template<typename _CharT, typename _Traits, typename _Alloc> _GLIBCXX20_CONSTEXPR - inline basic_string<_CharT, _Traits, _Alloc> + basic_string<_CharT, _Traits, _Alloc> operator+(const basic_string<_CharT, _Traits, _Alloc>& __lhs, - const _CharT* __rhs) - { - basic_string<_CharT, _Traits, _Alloc> __str(__lhs); - __str.append(__rhs); - return __str; - } + const _CharT* __rhs); /** * @brief Concatenate string and character. diff --git a/libstdc++-v3/include/bits/basic_string.tcc b/libstdc++-v3/include/bits/basic_string.tcc index 4563c61429a..95ba8e503e9 100644 --- a/libstdc++-v3/include/bits/basic_string.tcc +++ b/libstdc++-v3/include/bits/basic_string.tcc @@ -640,6 +640,27 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION return __str; } + template<typename _CharT, typename _Traits, typename _Alloc> + _GLIBCXX20_CONSTEXPR + basic_string<_CharT, _Traits, _Alloc> + operator+(const basic_string<_CharT, _Traits, _Alloc>& __lhs, + const _CharT* __rhs) + { + __glibcxx_requires_string(__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; + const __size_type __len = _Traits::length(__rhs); + __string_type __str(_Alloc_traits::_S_select_on_copy( + __lhs.get_allocator())); + __str.reserve(__len + __lhs.size()); + __str.append(__lhs); + __str.append(__rhs, __len); + return __str; + } + template<typename _CharT, typename _Traits, typename _Alloc> _GLIBCXX_STRING_CONSTEXPR typename basic_string<_CharT, _Traits, _Alloc>::size_type -- 2.34.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] libstdc++: Optimize operator+(string/char*, string/char*) equally 2022-08-24 6:16 ` [PATCH] libstdc++: Optimize operator+(string/char*,string/char*) equally whh8b 2022-08-24 6:16 ` [PATCH] libstdc++: Optimize operator+(string/char*, string/char*) equally whh8b @ 2022-08-24 14:24 ` Jonathan Wakely 2022-08-24 22:39 ` Alexandre Oliva 1 sibling, 1 reply; 13+ messages in thread From: Jonathan Wakely @ 2022-08-24 14:24 UTC (permalink / raw) To: whh8b; +Cc: libstdc++, gcc-patches On Wed, 24 Aug 2022 at 07:17, Will Hawkins wrote: > > 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. > > libstdc++-v3/ChangeLog: There should be a blank line here. > * libstdc++-v3/include/bits/basic_string.h (operator+(string, char*)): The path should be relative to the ChangeLog, so should not include the libstdc++-v3/ directory component. You can use the git gcc-verify alias to check your commit msgs format before submitting. That runs the same checks as will be used for the server-side hook that decides whether to allow a push. See the customization script described at https://gcc.gnu.org/gitwrite.html#vendor for the alaises. Also, the overload you're changing is operator+(const string&, const char*). The distinction matters, because there is also operator+(string&&, const char*) and what you wrote looks more like that one. So I've committed it with this changelog: libstdc++-v3/ChangeLog: * include/bits/basic_string.h (operator+(const string&, const char*)): Remove naive implementation. * include/bits/basic_string.tcc (operator+(const string&, const char*)): Add single-allocation implementation. Thanks for the patch! ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] libstdc++: Optimize operator+(string/char*, string/char*) equally 2022-08-24 14:24 ` Jonathan Wakely @ 2022-08-24 22:39 ` Alexandre Oliva 2022-08-24 22:47 ` Jonathan Wakely 0 siblings, 1 reply; 13+ messages in thread From: Alexandre Oliva @ 2022-08-24 22:39 UTC (permalink / raw) To: Jonathan Wakely via Gcc-patches; +Cc: whh8b, Jonathan Wakely, libstdc++ On Aug 24, 2022, Jonathan Wakely via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > * include/bits/basic_string.h (operator+(const string&, > const char*)): > Remove naive implementation. > * include/bits/basic_string.tcc (operator+(const string&, > const char*)): > Add single-allocation implementation. ISTM this requires the following additional tweak: diff --git a/libstdc++-v3/src/c++11/string-inst.cc b/libstdc++-v3/src/c++11/string-inst.cc index bfae6d902a1dd..2ec0e9d85f947 100644 --- a/libstdc++-v3/src/c++11/string-inst.cc +++ b/libstdc++-v3/src/c++11/string-inst.cc @@ -58,6 +58,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION template class basic_string<C>; template S operator+(const C*, const S&); + template S operator+(const S&, const C*); template S operator+(C, const S&); template S operator+(const S&, const S&); Without this, I'm getting undefined references to this specialization in libstdc++.so, that I tracked down to a std::system_error ctor in cxx11-ios_failure.o. I got this while testing another patch that might be the reason why the template instantiation doesn't get inlined, but... we can't depend on its being inlined, can we? -- Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ Free Software Activist GNU Toolchain Engineer Disinformation flourishes because many people care deeply about injustice but very few check the facts. Ask me about <https://stallmansupport.org> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] libstdc++: Optimize operator+(string/char*, string/char*) equally 2022-08-24 22:39 ` Alexandre Oliva @ 2022-08-24 22:47 ` Jonathan Wakely 2022-08-24 23:02 ` Jonathan Wakely 0 siblings, 1 reply; 13+ messages in thread From: Jonathan Wakely @ 2022-08-24 22:47 UTC (permalink / raw) To: Alexandre Oliva; +Cc: Jonathan Wakely via Gcc-patches, whh8b, libstdc++ On Wed, 24 Aug 2022 at 23:39, Alexandre Oliva <oliva@gnu.org> wrote: > > On Aug 24, 2022, Jonathan Wakely via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > > * include/bits/basic_string.h (operator+(const string&, > > const char*)): > > Remove naive implementation. > > * include/bits/basic_string.tcc (operator+(const string&, > > const char*)): > > Add single-allocation implementation. > > ISTM this requires the following additional tweak: > > diff --git a/libstdc++-v3/src/c++11/string-inst.cc b/libstdc++-v3/src/c++11/string-inst.cc > index bfae6d902a1dd..2ec0e9d85f947 100644 > --- a/libstdc++-v3/src/c++11/string-inst.cc > +++ b/libstdc++-v3/src/c++11/string-inst.cc > @@ -58,6 +58,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > template class basic_string<C>; > template S operator+(const C*, const S&); > + template S operator+(const S&, const C*); > template S operator+(C, const S&); > template S operator+(const S&, const S&); > > > Without this, I'm getting undefined references to this specialization in > libstdc++.so, that I tracked down to a std::system_error ctor in > cxx11-ios_failure.o. I got this while testing another patch that might > be the reason why the template instantiation doesn't get inlined, but... > we can't depend on its being inlined, can we? Right. But adding that will cause another symbol to be exported, probably with the wrong symbol version. To fix https://gcc.gnu.org/PR106735 I'm just going to revert it for now, and revisit in the morning. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] libstdc++: Optimize operator+(string/char*, string/char*) equally 2022-08-24 22:47 ` Jonathan Wakely @ 2022-08-24 23:02 ` Jonathan Wakely 2022-08-25 5:52 ` Will Hawkins 0 siblings, 1 reply; 13+ messages in thread From: Jonathan Wakely @ 2022-08-24 23:02 UTC (permalink / raw) To: Alexandre Oliva; +Cc: Jonathan Wakely via Gcc-patches, whh8b, libstdc++ On Wed, 24 Aug 2022 at 23:47, Jonathan Wakely <jwakely@redhat.com> wrote: > > On Wed, 24 Aug 2022 at 23:39, Alexandre Oliva <oliva@gnu.org> wrote: > > > > On Aug 24, 2022, Jonathan Wakely via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > > > > * include/bits/basic_string.h (operator+(const string&, > > > const char*)): > > > Remove naive implementation. > > > * include/bits/basic_string.tcc (operator+(const string&, > > > const char*)): > > > Add single-allocation implementation. > > > > ISTM this requires the following additional tweak: > > > > diff --git a/libstdc++-v3/src/c++11/string-inst.cc b/libstdc++-v3/src/c++11/string-inst.cc > > index bfae6d902a1dd..2ec0e9d85f947 100644 > > --- a/libstdc++-v3/src/c++11/string-inst.cc > > +++ b/libstdc++-v3/src/c++11/string-inst.cc > > @@ -58,6 +58,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > > > template class basic_string<C>; > > template S operator+(const C*, const S&); > > + template S operator+(const S&, const C*); > > template S operator+(C, const S&); > > template S operator+(const S&, const S&); > > > > > > Without this, I'm getting undefined references to this specialization in > > libstdc++.so, that I tracked down to a std::system_error ctor in > > cxx11-ios_failure.o. I got this while testing another patch that might > > be the reason why the template instantiation doesn't get inlined, but... > > we can't depend on its being inlined, can we? > > Right. But adding that will cause another symbol to be exported, > probably with the wrong symbol version. Oh, but those symbols aren't exported ... they're just needed because we compile with -fno-implicit-templatesand other symbols in libstdc++.so require them. So that probably is the right fix. I have another change for operator+ in mind now anyway, which optimizes operator(const string&, char) the same way, and removes the duplication in those five operator+ overloads. > > To fix https://gcc.gnu.org/PR106735 I'm just going to revert it for > now, and revisit in the morning. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] libstdc++: Optimize operator+(string/char*, string/char*) equally 2022-08-24 23:02 ` Jonathan Wakely @ 2022-08-25 5:52 ` Will Hawkins 0 siblings, 0 replies; 13+ messages in thread From: Will Hawkins @ 2022-08-25 5:52 UTC (permalink / raw) To: Jonathan Wakely Cc: Alexandre Oliva, Jonathan Wakely via Gcc-patches, libstdc++ On Wed, Aug 24, 2022 at 7:03 PM Jonathan Wakely <jwakely@redhat.com> wrote: > > On Wed, 24 Aug 2022 at 23:47, Jonathan Wakely <jwakely@redhat.com> wrote: > > > > On Wed, 24 Aug 2022 at 23:39, Alexandre Oliva <oliva@gnu.org> wrote: > > > > > > On Aug 24, 2022, Jonathan Wakely via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > > > > > > * include/bits/basic_string.h (operator+(const string&, > > > > const char*)): > > > > Remove naive implementation. > > > > * include/bits/basic_string.tcc (operator+(const string&, > > > > const char*)): > > > > Add single-allocation implementation. > > > > > > ISTM this requires the following additional tweak: > > > > > > diff --git a/libstdc++-v3/src/c++11/string-inst.cc b/libstdc++-v3/src/c++11/string-inst.cc > > > index bfae6d902a1dd..2ec0e9d85f947 100644 > > > --- a/libstdc++-v3/src/c++11/string-inst.cc > > > +++ b/libstdc++-v3/src/c++11/string-inst.cc > > > @@ -58,6 +58,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > > > > > template class basic_string<C>; > > > template S operator+(const C*, const S&); > > > + template S operator+(const S&, const C*); > > > template S operator+(C, const S&); > > > template S operator+(const S&, const S&); > > > I realize that I should have noticed that asymmetry as well. Sorry! > > > > > > Without this, I'm getting undefined references to this specialization in > > > libstdc++.so, that I tracked down to a std::system_error ctor in > > > cxx11-ios_failure.o. I got this while testing another patch that might > > > be the reason why the template instantiation doesn't get inlined, but... > > > we can't depend on its being inlined, can we? > > > > Right. But adding that will cause another symbol to be exported, > > probably with the wrong symbol version. > > Oh, but those symbols aren't exported ... they're just needed because > we compile with -fno-implicit-templatesand other symbols in > libstdc++.so require them. > > So that probably is the right fix. I have another change for operator+ > in mind now anyway, which optimizes operator(const string&, char) the > same way, and removes the duplication in those five operator+ > overloads. Let me know if/how I can help. Will > > > > > To fix https://gcc.gnu.org/PR106735 I'm just going to revert it for > > now, and revisit in the morning. > ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-08-25 5:52 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-22 18:15 [PATCH] libstdc++: Optimize operator+(string/char*, string/char*) equally whh8b 2022-08-22 18:15 ` whh8b 2022-08-23 16:33 ` Jonathan Wakely 2022-08-24 6:18 ` Will Hawkins 2022-08-24 9:49 ` Jonathan Wakely 2022-08-24 6:16 ` [PATCH v2] libstdc++: Optimize operator+(string/char*,string/char*) whh8b 2022-08-24 6:16 ` [PATCH] libstdc++: Optimize operator+(string/char*,string/char*) equally whh8b 2022-08-24 6:16 ` [PATCH] libstdc++: Optimize operator+(string/char*, string/char*) equally whh8b 2022-08-24 14:24 ` Jonathan Wakely 2022-08-24 22:39 ` Alexandre Oliva 2022-08-24 22:47 ` Jonathan Wakely 2022-08-24 23:02 ` Jonathan Wakely 2022-08-25 5:52 ` Will Hawkins
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).