From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 79724 invoked by alias); 20 Sep 2016 08:58:01 -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 79132 invoked by uid 89); 20 Sep 2016 08:58:00 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.7 required=5.0 tests=BAYES_50,RP_MATCHES_RCVD,SPF_HELO_PASS,TBC autolearn=ham version=3.3.2 spammy=UD:B, N.B, UD:N.B, n.b X-Spam-User: qpsmtpd, 2 recipients X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 20 Sep 2016 08:57:50 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 362F14E041; Tue, 20 Sep 2016 08:57:49 +0000 (UTC) Received: from localhost (ovpn-116-66.ams2.redhat.com [10.36.116.66]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u8K8vmUG022348; Tue, 20 Sep 2016 04:57:48 -0400 Date: Tue, 20 Sep 2016 09:20:00 -0000 From: Jonathan Wakely To: =?iso-8859-1?Q?Fran=E7ois?= Dumont Cc: "libstdc++@gcc.gnu.org" , gcc-patches Subject: Re: debug container mutex association Message-ID: <20160920085747.GD17376@redhat.com> References: <46b4f297-c0da-b0b0-03ef-bba019b97ec7@gmail.com> <20160914090036.GC17376@redhat.com> <20160915085143.GJ17376@redhat.com> <2af7879c-bef5-7874-b5ae-722ad3a1e171@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <2af7879c-bef5-7874-b5ae-722ad3a1e171@gmail.com> X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.7.0 (2016-08-17) X-SW-Source: 2016-09/txt/msg01264.txt.bz2 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.