From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 77050 invoked by alias); 20 Jun 2019 20:40:42 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 77028 invoked by uid 89); 20 Jun 2019 20:40:41 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=remind, size_type, __n, __catch X-HELO: mail-wr1-f68.google.com Received: from mail-wr1-f68.google.com (HELO mail-wr1-f68.google.com) (209.85.221.68) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 20 Jun 2019 20:40:37 +0000 Received: by mail-wr1-f68.google.com with SMTP id n4so3166364wrs.3; Thu, 20 Jun 2019 13:40:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language; bh=giSSwDFrRmCgMQ0mrGAMQVZVLX5dmpw2kC0vPPVklrA=; b=A1pFEmB4oNMCxyHfWhZdbeaKlgaAyRR6GeUp5id5amDkEstngN84FovOuMnrMpoj2D HnFUkHZXREe0DdSSVeyMm740NXqrRoKT5egwyyAP1bCljBtxWjs3Zg0z5ezR7+TCz0NG tyusH+Zs5YMUf1lnswo6tlQDX4Gv2WwpFg3vXySyTlU+fqsuVQZ0Sbnj3lLuvblAsRx5 AN1DH/unu5c1EiY/UshmbwkWwnvM9NgP0gmeBjKa39eKPlkAP79Ji8FmGQlbehGGELRq kVEkbDPOI5HCNBiC57L+JzF+uL8o0mfFWGFhX8FNuf32QCGkr+8aLiICo8TYNJM51vvE QGtQ== Return-Path: Received: from ?IPv6:2a01:e0a:1dc:b1c0:5198:a8cf:6661:3ddc? ([2a01:e0a:1dc:b1c0:5198:a8cf:6661:3ddc]) by smtp.googlemail.com with ESMTPSA id y6sm759287wrp.12.2019.06.20.13.40.33 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 20 Jun 2019 13:40:33 -0700 (PDT) Subject: Re: Review Hashtable extract node API To: Jonathan Wakely Cc: "libstdc++@gcc.gnu.org" , gcc-patches References: <0a927e9b-97d5-5fc8-fb36-2af5848e017b@gmail.com> <20190605162245.GK2599@redhat.com> <05a12fbd-923a-7290-e12c-1694a25fc22a@gmail.com> <20190617102829.GQ643@redhat.com> <20190618105409.GU643@redhat.com> <9c94f9ed-c873-456f-b7c1-87e7b2577745@gmail.com> <20190618224758.GC7627@redhat.com> From: =?UTF-8?Q?Fran=c3=a7ois_Dumont?= Message-ID: Date: Thu, 20 Jun 2019 20:40:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 MIME-Version: 1.0 In-Reply-To: <20190618224758.GC7627@redhat.com> Content-Type: multipart/mixed; boundary="------------4A1EE880ED82B4507DB6346D" X-SW-Source: 2019-06/txt/msg01268.txt.bz2 This is a multi-part message in MIME format. --------------4A1EE880ED82B4507DB6346D Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-length: 2681 On 6/19/19 12:47 AM, Jonathan Wakely wrote: > On 18/06/19 22:42 +0200, François Dumont wrote: >> On 6/18/19 12:54 PM, Jonathan Wakely wrote: >>> On 18/06/19 07:52 +0200, François Dumont wrote: >>>> A small regression noticed while merging. >>>> >>>> We shouldn't keep on using a moved-from key_type instance. >>>> >>>> Ok to commit ? Feel free to do it if you prefer, I'll do so at end >>>> of Europe day otherwise. >>> >>> >>>> diff --git a/libstdc++-v3/include/bits/hashtable_policy.h >>>> b/libstdc++-v3/include/bits/hashtable_policy.h >>>> index f5809c7443a..7e89e1b44c4 100644 >>>> --- a/libstdc++-v3/include/bits/hashtable_policy.h >>>> +++ b/libstdc++-v3/include/bits/hashtable_policy.h >>>> @@ -743,7 +743,8 @@ namespace __detail >>>>     std::tuple<>() >>>>       }; >>>>       auto __pos >>>> -    = __h->_M_insert_unique_node(__k, __bkt, __code, __node._M_node); >>>> +    = >>>> __h->_M_insert_unique_node(__h->_M_extract()(__node._M_node->_M_v()), >>>> +                     __bkt, __code, __node._M_node); >>>>       __node._M_node = nullptr; >>>>       return __pos->second; >>>>     } >>> >>> I can't create an example where this causes a problem, because the key >>> passed to _M_insert_unique_node is never used. So it doesn't matter >>> that it's been moved from. >>> >>> So I have to wonder why we just added the key parameter to that >>> function, if it's never used. >> >> I think you've been influence by my patch. I was using a >> "_NodeAccessor" which wasn't giving access to the node without taking >> owership so I needed to pass the key properly to compute new bucket >> index in case of rehash. >> >> But with your approach this change to the _M_insert_unique_node was >> simply unecessary so here is a patch to cleanup this part. > > Ha! I see, thanks. So I should have removed that key_type parameter > again after removing the NodeAccessor stuff. > > >> Ok to commit ? > > No, because that would restore the original signature of the > _M_insert_unique_node function, but it has changed contract. Old > callers who expect that function to delete the node would now leak > memory if an exception is thrown. > Oh, yes, abi, I tend to forget even if the recent PR 90920 remind me about that, sorry. > If we change the contract of the function we need to change its > mangled name, so that callers expecting the old contract will not use > the new function. > > I'll think about the best way to do that ... > Something like what's attached ? I still use _GLIBCXX_INLINE_VERSION to tag functions that are kept just for abi-compatibility. Ok to commit after having run tests ? François --------------4A1EE880ED82B4507DB6346D Content-Type: text/x-patch; name="hashtable.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="hashtable.patch" Content-length: 8792 diff --git a/libstdc++-v3/include/bits/hashtable.h b/libstdc++-v3/include/bits/hashtable.h index ab579a7059e..2ea75a24f1c 100644 --- a/libstdc++-v3/include/bits/hashtable.h +++ b/libstdc++-v3/include/bits/hashtable.h @@ -693,19 +693,35 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __node_base* _M_get_previous_node(size_type __bkt, __node_base* __n); - // Insert node __n with key __k and hash code __code, in bucket __bkt - // if no rehash (assumes no element with same key already present). + // Insert node __n with hash code __code, in bucket __bkt if no + // rehash (assumes no element with same key already present). // Takes ownership of __n if insertion succeeds, throws otherwise. iterator - _M_insert_unique_node(const key_type& __k, size_type __bkt, - __hash_code __code, __node_type* __n, - size_type __n_elt = 1); + _M_insert_node(true_type, size_type __bkt, __hash_code, + __node_type* __n, size_type __n_elt = 1); + +#if !_GLIBCXX_INLINE_VERSION + // Insert node with hash code __code, in bucket bkt if no rehash (assumes + // no element with its key already present). Take ownership of the node, + // deallocate it on exception. + iterator + _M_insert_unique_node(size_type __bkt, __hash_code __code, + __node_type* __n, size_type __n_elt = 1); +#endif // Insert node __n with key __k and hash code __code. // Takes ownership of __n if insertion succeeds, throws otherwise. iterator - _M_insert_multi_node(__node_type* __hint, const key_type& __k, + _M_insert_node(false_type, __node_type* __hint, + __hash_code __code, __node_type* __n); + +#if !_GLIBCXX_INLINE_VERSION + // Insert node with hash code __code. Take ownership of the node, + // deallocate it on exception. + iterator + _M_insert_multi_node(__node_type* __hint, __hash_code __code, __node_type* __n); +#endif template std::pair @@ -831,7 +847,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION else { __ret.position - = _M_insert_unique_node(__k, __bkt, __code, __nh._M_ptr); + = _M_insert_node(true_type{}, __bkt, __code, __nh._M_ptr); __nh._M_ptr = nullptr; __ret.inserted = true; } @@ -851,7 +867,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION const key_type& __k = __nh._M_key(); auto __code = this->_M_hash_code(__k); auto __ret - = _M_insert_multi_node(__hint._M_cur, __k, __code, __nh._M_ptr); + = _M_insert_node(false_type{}, __hint._M_cur, __code, __nh._M_ptr); __nh._M_ptr = nullptr; return __ret; } @@ -918,8 +934,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION if (_M_find_node(__bkt, __k, __code) == nullptr) { auto __nh = __src.extract(__pos); - _M_insert_unique_node(__k, __bkt, __code, __nh._M_ptr, - __n_elt); + _M_insert_node(true_type{}, __bkt, __code, __nh._M_ptr, __n_elt); __nh._M_ptr = nullptr; __n_elt = 1; } @@ -1671,7 +1686,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION return std::make_pair(iterator(__p), false); // Insert the node - auto __pos = _M_insert_unique_node(__k, __bkt, __code, __node._M_node); + auto __pos = _M_insert_node(true_type{}, __bkt, __code, __node._M_node); __node._M_node = nullptr; return { __pos, true }; } @@ -1693,7 +1708,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __hash_code __code = this->_M_hash_code(__k); auto __pos - = _M_insert_multi_node(__hint._M_cur, __k, __code, __node._M_node); + = _M_insert_node(false_type{}, __hint._M_cur, __code, __node._M_node); __node._M_node = nullptr; return __pos; } @@ -1705,9 +1720,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION auto _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, _H1, _H2, _Hash, _RehashPolicy, _Traits>:: - _M_insert_unique_node(const key_type& __k, size_type __bkt, - __hash_code __code, __node_type* __node, - size_type __n_elt) + _M_insert_node(true_type, size_type __bkt, __hash_code __code, + __node_type* __node, size_type __n_elt) -> iterator { const __rehash_state& __saved_state = _M_rehash_policy._M_state(); @@ -1718,7 +1732,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION if (__do_rehash.first) { _M_rehash(__do_rehash.second, __saved_state); - __bkt = _M_bucket_index(__k, __code); + __bkt = _M_bucket_index(this->_M_extract()(__node->_M_v()), __code); } this->_M_store_code(__node, __code); @@ -1729,6 +1743,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION return iterator(__node); } +#if !_GLIBCXX_INLINE_VERSION template:: - _M_insert_multi_node(__node_type* __hint, const key_type& __k, - __hash_code __code, __node_type* __node) + _M_insert_unique_node(size_type __bkt, __hash_code __code, + __node_type* __node, size_type __n_elt) + -> iterator + { + __try + { + return _M_insert_node(true_type{}, __bkt, __code, __node, __n_elt); + } + __catch + { + this->_M_deallocate_node(__node); + __throw_exception_again; + } + } +#endif + + template + auto + _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, + _H1, _H2, _Hash, _RehashPolicy, _Traits>:: + _M_insert_node(false_type, __node_type* __hint, + __hash_code __code, __node_type* __node) -> iterator { const __rehash_state& __saved_state = _M_rehash_policy._M_state(); @@ -1748,6 +1786,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _M_rehash(__do_rehash.second, __saved_state); this->_M_store_code(__node, __code); + const key_type& __k = this->_M_extract()(__node->_M_v()); size_type __bkt = _M_bucket_index(__k, __code); // Find the node before an equivalent one or use hint if it exists and @@ -1782,6 +1821,30 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION return iterator(__node); } +#if !_GLIBCXX_INLINE_VERSION + template + auto + _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, + _H1, _H2, _Hash, _RehashPolicy, _Traits>:: + _M_insert_multi_node(__node_type* __hint, + __hash_code __code, __node_type* __node) + -> iterator + { + __try + { + return _M_insert_node(false_type{}, __hint, __code, __node); + } + __catch + { + this->_M_deallocate_node(__node); + __throw_exception_again; + } + } +#endif + // Insert v if no element with its key is already present. template(__v)), this }; auto __pos - = _M_insert_unique_node(__k, __bkt, __code, __node._M_node, __n_elt); + = _M_insert_node(true_type{}, __bkt, __code, __node._M_node, __n_elt); __node._M_node = nullptr; return { __pos, true }; } @@ -1830,7 +1893,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _Scoped_node __node{ __node_gen(std::forward<_Arg>(__v)), this }; const key_type& __k = this->_M_extract()(__node._M_node->_M_v()); auto __pos - = _M_insert_multi_node(__hint._M_cur, __k, __code, __node._M_node); + = _M_insert_node(false_type{}, __hint._M_cur, __k, __code, + __node._M_node); __node._M_node = nullptr; return __pos; } diff --git a/libstdc++-v3/include/bits/hashtable_policy.h b/libstdc++-v3/include/bits/hashtable_policy.h index f5809c7443a..6bb59e2be3a 100644 --- a/libstdc++-v3/include/bits/hashtable_policy.h +++ b/libstdc++-v3/include/bits/hashtable_policy.h @@ -716,7 +716,7 @@ namespace __detail std::tuple<>() }; auto __pos - = __h->_M_insert_unique_node(__k, __bkt, __code, __node._M_node); + = __h->_M_insert_node(true_type{}, __bkt, __code, __node._M_node); __node._M_node = nullptr; return __pos->second; } @@ -743,7 +743,7 @@ namespace __detail std::tuple<>() }; auto __pos - = __h->_M_insert_unique_node(__k, __bkt, __code, __node._M_node); + = __h->_M_insert_node(true_type{}, __bkt, __code, __node._M_node); __node._M_node = nullptr; return __pos->second; } --------------4A1EE880ED82B4507DB6346D--