From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 894 invoked by alias); 30 Jul 2009 08:46:39 -0000 Received: (qmail 879 invoked by uid 22791); 30 Jul 2009 08:46:37 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 30 Jul 2009 08:46:28 +0000 Received: (qmail 5274 invoked from network); 30 Jul 2009 08:46:26 -0000 Received: from unknown (HELO wind.localnet) (vladimir@127.0.0.2) by mail.codesourcery.com with ESMTPA; 30 Jul 2009 08:46:26 -0000 From: Vladimir Prus To: Jan Kratochvil Subject: Re: [patch 0/4] varobj_list replacement [Re: [patch 4/8] Types GC [varobj_list to all_root_varobjs]] Date: Thu, 30 Jul 2009 15:13:00 -0000 User-Agent: KMail/1.11.90 (Linux/2.6.24-24-generic; KDE/4.2.90; i686; svn-979530; 2009-06-10) Cc: Tom Tromey , gdb-patches@sourceware.org References: <20090525080233.GD13323@host0.dyn.jankratochvil.net> <200907301045.51541.vladimir@codesourcery.com> <20090730075220.GA14542@host0.dyn.jankratochvil.net> In-Reply-To: <20090730075220.GA14542@host0.dyn.jankratochvil.net> MIME-Version: 1.0 Content-Type: Text/Plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Message-Id: <200907301246.25219.vladimir@codesourcery.com> Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-07/txt/msg00738.txt.bz2 On Thursday 30 July 2009 Jan Kratochvil wrote: > On Thu, 30 Jul 2009 08:45:51 +0200, Vladimir Prus wrote: > > On Wednesday 29 July 2009 Tom Tromey wrote: > > > I understand from other mail that this patch is a prerequisite to the > > > type GC work. However, I don't understand in what way it is needed. I > > > probably missed something... could you either explain it or tell me > > > where to look? > > > > In fact, I'm lost the big picture as well. If we want to optimize uninstall_variable, > > then the 4/4 patch appears to be the simplest one that does the trick. However, if > > that's a part of some bigger story, I'd be interested to understand it. > > The bigger story is: > ------------------------------------------------------------------------------ > The former Types GC code had to traverse varobj leaves (not just roots as does > current varobj_list()). As I found the current varobj_list() calling > convention is IMO-inconvenient and calling convention of the 'roots' and 'all' > iterators/enumerators of varobjs should be the same I did not want to write > a new function using the IMO-inconvenient calling convention. The new > function was called all_varobjs() in the obsoleted patch: > [patch 8/8] Types GC [varobj] > http://sourceware.org/ml/gdb-patches/2009-05/msg00551.html > So I rather IMO-fixed the calling convention of varobj_list() first so that > the later 'all' varobjs iterator can already use the new calling convention > while keeping the calling convention of the 'roots' and 'all' iterators the > same. > > I found the new all_root_varobjs() iterator - patch 3/4 - to be clearly a win > over current varobj_list() - no matter how insignificant change it may be > - that just nobody so far has spent the time doing such code cleanup. > The patches 1/4, 2/4 and 4/4 were later created just to support accepting the > patch 3/4 and I probably would not submit 3/4 if I could imagine any problems > getting it accepted. > > Moreover the Types GC changed from the former reference-counting to the > current mark-and-sweep where maybe the leaves traversal is no longer needed. > I have to check it more. > > speculation: IIRC it should have been fixing one objfile-invalidating bug > where varobj leaf is using a different objfile due to TYPE_STUB referencing. > But it is a bug out of the critical path for Types GC. > > In fact I do not like 1/4 or 2/4 to be accepted, they were created just to get > 3/4 accepted. Also I do not think the 4/4 performance improvement is worth > the reviewing time, it was also created just to get 3/4 accepted. I was confused -- I though 4/4 is meant is the preferred one. > As I see 3/4 is not considered as a clear cleanup win. Understood it as > rejected and I will be basing the next patches on the current varobj_list() > calling convention as the preferred one. In fact, varobj_list() is surely messy, I was not sure if the callback-iteration is better than vec. Of course, this question might be easier solved were we using C++, which has both a pile of data types and iterators (non-callback-based). However, we've probably spent too much time over matter than can be easily tweaked in future. If your current patches depend on 3/4, please commit it -- no need to rework things over that. - Volodya