Re: debug container mutex association

[Jonathan Wakely <jwakely at redhat dot com>]

On 19/09/16 21:56 +0200, François Dumont wrote:
> Hi
>
>    Following our conversation here is a much simpler patch. I just 
> consider that all debug containers will have the same alignment.
>
>    Even if I submit this patch as a whole I will commit into pieces, 
> at least one for the pure cleanup parts and one for the debug.cc 
> change.
>
>    Among those changes there is:
> -       __gnu_cxx::__scoped_lock(this->_M_get_mutex());
> +       __gnu_cxx::__scoped_lock __l(this->_M_get_mutex());
>
>    I would appreciate if you could tell me what was happening with 
> the initial expression. I don't understand why it is compiling. I even 
> tried the same in debug.cc and started having compilation errors.
>
>    * include/debug/bitset (bitset::reference::reference(const _Base_ref&,
>    bitset*)): Remove __unused__ attribute.
>    * include/debug/safe_base.h (_Safe_iterator_base): Make
>    _Safe_sequence_base a friend.
>    (_Safe_iterator_base::_M_attach): Make protected.
>    (_Safe_iterator_base::_M_attach_single): Likewise.
>    (_Safe_iterator_base::_M_detach): Likewise.
>    (_Safe_iterator_base::_M_detach_single): Likewise.
>    (_Safe_sequence_base): Make _Safe_iterator_base a friend.
> (_Safe_sequence_base::_Safe_sequence_base(_Safe_sequence_base&&)): New.
>    (_Safe_sequence_base::_M_swap): Make protected.
>    (_Safe_sequence_base::_M_attach): Make private.
>    (_Safe_sequence_base::_M_attach_single): Likewise.
>    (_Safe_sequence_base::_M_detach): Likewise.
>    (_Safe_sequence_base::_M_detach_single): Likewise.
>    * include/debug/safe_container.h
>    (_Safe_container::_Safe_container(_Safe_container&&)): Make default.
>    * include/debug/safe_iterator.h
>    (_Safe_iterator::operator++()): Name __scoped_lock instance.
>    * include/debug/safe_iterator.tcc: Remove trailing line.
>    * include/debug/safe_unordered_base.h
>    (_Safe_local_iterator_base::_M_attach): Make protected.
>    (_Safe_local_iterator_base::_M_attach_single): Likewise.
>    (_Safe_local_iterator_base::_M_detach): Likewise.
>    (_Safe_local_iterator_base::_M_detach_single): Likewise.
>    (_Safe_unordered_container_base): Make _Safe_local_iterator_base 
> friend.
>    (_Safe_unordered_container_base::_M_attach_local): Make private.
>    (_Safe_unordered_container_base::_M_attach_local_single): Likewise.
>    (_Safe_unordered_container_base::_M_detach_local): Likewise.
>    (_Safe_unordered_container_base::_M_detach_local_single): Likewise.
>    * src/c++11/debug.cc: Include debug/vector. Include cctype. Remove
>    functional.
>    (get_safe_base_mutex): Get mutex based on address lowest non nil bits.
>    * testsuite/23_containers/vector/debug/mutex_association.cc: New.
>
> Tested under Linux x86_64.
>
> Ok to commit ?

Yes, OK for trunk.  Thanks for revising it, I think this is a much
bettter fix.


> On 15/09/2016 10:51, Jonathan Wakely wrote:
> > N.B. https://gcc.gnu.org/PR71312
> > > Ok, debug mode is also impacted. Shouldn't the alignment be set on 
> > > the __mutex type directly ?
> >
> > No, definitely not. Apart from the fact that it would be an ABI
> > change, it would mean that struct X { __mutex mx; int i; } would not
> > place the int right next to the mutex, it would force it onto a
> > different cacheline. We want our arrays of mutexes to be on separate
> > cachelines, but most uses of __mutex are not in an array.
> >
> > We probably can't fix this properly yet, because we don't have the
> > hardware_destructive_interference_size value. We could just make a
> > conservative estimate of 64 bytes though.
> Thanks for explaining that it is to avoid false sharing.
>
> Maybe we could share this mutex pool with debug mode. This way the day 
> we fix this false sharing issue it will benefit to both shared_ptr and 
> debug mode.

Yes, that would reduce the size of the library, by not having two
arrays of mutexes. Currently the arrays are not visible outside their
own file, but that's not too hard to solve.