This is the mail archive of the libstdc++@gcc.gnu.org mailing list for the libstdc++ project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] fix libstdc++/56267 - local iterator requirements


On 01/20/2014 04:53 PM, Jonathan Wakely wrote:
On 17 January 2014 15:11, Jonathan Wakely wrote:
The issue in PR 56267 is that unordered_xxx::local_iterator sometimes
inherits from the user-defined hash function (via _Hash_code_base,
which inherits from the hash function to use the EBO), and
local_iterator must be DefaultConstructible and Assignable, even when
the hash function isn't.

My solution is to remove the inheritance from _Hash_code_base, and
instead construct/destroy the _Hash_code_base in a block of
uninitialized memory (via __gnu_cxx::__aligned_buffer). This would
mean we can't use the EBO and increase the size of local_iterator, and
past measurements have shown that the unordered containers'
performance is sensitive to such changes, so there's a partial
specialization that doesn't have the __aligned_buffer member for the
case where the _Hash_code_base is empty and needs no storage.

François, do you have any comments on this? Can you see a better solution?

While working on this I decided I didn't like everything in
_Local_iterator_base being public, so I added some accessors to the
only members that are needed by unrelated types.
Tested x86_64-linux and committed to trunk.

2014-01-20  Jonathan Wakely  <jwakely@redhat.com>

         PR libstdc++/56267
         * include/bits/hashtable_policy.h (_Hash_code_base<... false>): Grant
         friendship to _Local_iterator_base<..., false>.
         (_Local_iterator_base): Give protected access to all existing members.
         (_Local_iterator_base::_M_curr()): New public accessor.
         (_Local_iterator_base::_M_get_bucket()): New public accessor.
         (_Local_iterator_base<..., false>::_M_init()): New function to manage
         the lifetime of the _Hash_code_base explicitly.
         (_Local_iterator_base<..., false>::_M_destroy()): Likewise.
         (_Local_iterator_base<..., false>): Define copy constructor and copy
         assignment operator that use new functions to manage _Hash_code_base.
         (operator==(const _Local_iterator_base&, const _Local_iterator_base&),
         operator==(const _Local_iterator_base&, const _Local_iterator_base&)):
         Use public API for _Local_iterator_base.
         * include/debug/safe_local_iterator.h (_Safe_local_iterator): Likewise.
         * include/debug/unordered_map (__debug::unordered_map::erase(),
         __debug::unordered_multimap::erase()): Likewise.
         * include/debug/unordered_set (__debug::unordered_set::erase(),
         __debug::unordered_multiset::erase()): Likewise.
         * testsuite/23_containers/unordered_set/56267-2.cc: New test.


I considered that it was an acceptable constraint on the hash functor but if it is not Standard compliant then the solution is good and I cannot think about another one.

With this new design couldn't we change the conditions that are used to decide when to cache the hash code. I haven't study it in detail but it looks like the default constructible constraint could be removed, no ?

François


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]