From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 167ED3858292 for ; Tue, 8 Nov 2022 17:44:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 167ED3858292 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1667929455; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=WXOShNNQuJXYC4Z0sehWy65vyX1e5hczCdFKhktcxo0=; b=iIhR6QBwY3xUEqI2crgE+ZHe/LGBawX5z02t6wCSeJFHJ4IjDDX77VnfANmriOM7ZKc1l1 cc/HzO746mPewY3b7dkTo6e+IJR/cTCY1MQAgYtcayBeOFmToskAY25Q8BQ2jNJdpPbyvh c9r8YhfGKl6qoc69VjLwVBAgNTfXxi0= Received: from mail-ed1-f71.google.com (mail-ed1-f71.google.com [209.85.208.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-655-NkrgWx6rOp2fP3gULs32iA-1; Tue, 08 Nov 2022 12:44:14 -0500 X-MC-Unique: NkrgWx6rOp2fP3gULs32iA-1 Received: by mail-ed1-f71.google.com with SMTP id y18-20020a056402359200b004635f8b1bfbso11066238edc.17 for ; Tue, 08 Nov 2022 09:44:14 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=WXOShNNQuJXYC4Z0sehWy65vyX1e5hczCdFKhktcxo0=; b=QqknM2dHQjBaQwkZLlPDOHtHtG7nqXqxBSfdPdwUeWBJ6DZNAc+WxQt3hz/rWULkrt 5zQu0q+OYgiuO5b5j94Hau6szVBM9BEjFzPH7z4y64E+175saWlZWYAo/TXwadNp4zcw IFEWtkUVL6sGJaG9uvOKeCir1enl2SW5saL5GZI4T6Q2bG05t6fr+SUxHPPiJoilm+zN mKKVUw3gudKmZ7nkn3GfGwKyMgE47n3ratn6rBpGSuUx+tPwBzLUUjive8SptgWNQVoF XOK8MmgiIQBOJRBTDKIc7m84YcAOP42HRapf6zcYEVNZ3XLkjQoYzFVnqURr+Dc+J8wt TKWQ== X-Gm-Message-State: ACrzQf0uicPvQCIYV9ju0VxAdaccUIlw1FJLWOQ1RL14tCJCU6B7HUu8 CEcVEEsbSkcYPKPXGv2nmLSBEt/j3ygN1AGHrVsgTeJ4mDX0whOynpRAp3CHK4SUu/ObcPLN4dM E0ZE8gtJ7mNPfk+TIUPx/O1JiSEB1OyE= X-Received: by 2002:a17:906:9bcd:b0:7ae:2679:c47 with SMTP id de13-20020a1709069bcd00b007ae26790c47mr25072639ejc.353.1667929453345; Tue, 08 Nov 2022 09:44:13 -0800 (PST) X-Google-Smtp-Source: AMsMyM4zK/NB0aYDgbnt5LIBLBNAPol8xh263Y2vVeCx4T1qmp5UGByLYUXo3kmowoUjPtdXNr+9b9/in+GvBqXYXzE= X-Received: by 2002:a17:906:9bcd:b0:7ae:2679:c47 with SMTP id de13-20020a1709069bcd00b007ae26790c47mr25072620ejc.353.1667929453030; Tue, 08 Nov 2022 09:44:13 -0800 (PST) MIME-Version: 1.0 References: <20221020000559.371886-1-whh8b@obs.cr> In-Reply-To: <20221020000559.371886-1-whh8b@obs.cr> From: Jonathan Wakely Date: Tue, 8 Nov 2022 17:44:02 +0000 Message-ID: Subject: Re: [PATCH] libstdc++: Refactor implementation of operator+ for std::string To: Will Hawkins Cc: gcc-patches@gcc.gnu.org, libstdc++@gcc.gnu.org X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-12.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 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 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 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 > + _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 _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 > _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 > _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 > _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 > - _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 > - _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 > _GLIBCXX_STRING_CONSTEXPR > typename basic_string<_CharT, _Traits, _Alloc>::size_type > -- > 2.37.3 >