From: Jonathan Wakely <jwakely@redhat.com>
To: "François Dumont" <frs.dumont@gmail.com>
Cc: "libstdc++@gcc.gnu.org" <libstdc++@gcc.gnu.org>,
gcc-patches <gcc-patches@gcc.gnu.org>
Subject: Re: [patch] libstdc++/61143 make unordered containers usable after move
Date: Tue, 20 May 2014 19:36:00 -0000 [thread overview]
Message-ID: <20140520193614.GA5959@redhat.com> (raw)
In-Reply-To: <537A693C.9040904@gmail.com>
On 19/05/14 22:27 +0200, François Dumont wrote:
>On 15/05/2014 22:52, Jonathan Wakely wrote:
>>Does this get initialized in the constructors?
>>Would it make sense to give it an initializer?
>>
>> __bucket_type _M_single_bucket = nullptr;
>
> This bucket is replacing those normally allocated and when they
>are allocated they are 0 initialised. So, you were right, there were
>one place where this initialization was missing which is fixed in this
>new patch. So I don't think this additional initialization is
>necessary.
OK
>>>@@ -980,12 +999,16 @@
>>> _M_move_assign(_Hashtable&& __ht, std::true_type)
>>> {
>>> this->_M_deallocate_nodes(_M_begin());
>>>- if (__builtin_expect(_M_bucket_count != 0, true))
>>>- _M_deallocate_buckets();
>>>-
>>>+ _M_deallocate_buckets();
>>> __hashtable_base::operator=(std::move(__ht));
>>> _M_rehash_policy = __ht._M_rehash_policy;
>>>- _M_buckets = __ht._M_buckets;
>>>+ if (__builtin_expect(__ht._M_buckets !=
>>>&__ht._M_single_bucket, true))
>>>+ _M_buckets = __ht._M_buckets;
>>
>>What is the value of this->_M_single_bucket now?
>>
>>Should it be set to nullptr, if only to help debugging?
>
>We are not resetting buckets to null when rehashing so unless I add
>more checks I won't be able to reset it each time.
OK
>>
>>>+ if (__builtin_expect(__ht._M_buckets ==
>>>&__ht._M_single_bucket, false))
>>
>>This check appears in a few places, I wonder if it is worth creating a
>>private member function to hide the details:
>>
>> bool _M_moved_from() const noexcept
>> {
>> return __builtin_expect(_M_buckets == &_M_single_bucket, false);
>> }
>>
>>Then just test if (__ht._M_moved_from())
>>
>>Usually I would think the __builtin_expect should not be inside the
>>member function, so the caller decides what the expected result is,
>>but I think in all cases the result is expected to be false. That
>>matches how move semantics are designed: the object that gets moved
>>from is expected to be going out of scope, and so will be reused in a
>>minority of cases.
>>
>>>@@ -1139,7 +1170,14 @@
>>> {
>>> if (__ht._M_node_allocator() == this->_M_node_allocator())
>>> {
>>>- _M_buckets = __ht._M_buckets;
>>>+ if (__builtin_expect(__ht._M_buckets ==
>>>&__ht._M_single_bucket, false))
>>
>>This could be if (__ht._M_moved_from())
>
>I hesitated in doing so and finally do so. I only prefer
>_M_use_single_bucket as we might not limit its usage to moved
>instances.
Good point.
I think it should be called _M_uses_single_bucket() or
_M_using_single_bucket() though, otherwise it sounds more like it
answers the question "should I use a single bucket?" rather than "am I
using the single bucket?"
>>How about removing the std::swap(_M_buckets, __x._M_buckets) above and
>>doing (untested):
>>
>> if (this->_M_moved_from())
>> {
>> if (__x._M_moved_from())
>> _M_buckets = &_M_single_bucket;
>> else
>> _M_buckets = __x._M_buckets;
>> __x._M_buckets = &__x._M_single_bucket;
>> }
>> else if (__x._M_moved_from())
>> {
>> __x._M_buckets = _M_buckets;
>> _M_buckets = &_M_single_bucket;
>> }
>> else
>> std::swap(_M_buckets, __x._M_buckets);
>>
>>Is that worth it? I'm not sure.
>
>Yes, with the newly introduced _M_use_single_bucket it worth it I
>think and is what I have done.
OK. My sketch above avoided calling _M_moved_from() more than once per
object, but the compiler should be able to optimise your version to
avoid multiple calls anyway.
>Here is the new patch limited to what I really want to commit this time.
Great. Please commit to trunk and the 4.9 branch - thanks!
next prev parent reply other threads:[~2014-05-20 19:36 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-05-15 20:20 François Dumont
2014-05-15 20:31 ` Paolo Carlini
2014-05-15 20:52 ` Jonathan Wakely
2014-05-19 20:27 ` François Dumont
2014-05-20 19:36 ` Jonathan Wakely [this message]
2014-05-20 20:37 ` François Dumont
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=20140520193614.GA5959@redhat.com \
--to=jwakely@redhat.com \
--cc=frs.dumont@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=libstdc++@gcc.gnu.org \
/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).