This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch 0/4] varobj_list replacement [Re: [patch 4/8] Types GC [varobj_list to all_root_varobjs]]
- From: Vladimir Prus <vladimir at codesourcery dot com>
- To: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- Cc: Tom Tromey <tromey at redhat dot com>, gdb-patches at sourceware dot org
- Date: Thu, 30 Jul 2009 12:46:24 +0400
- Subject: Re: [patch 0/4] varobj_list replacement [Re: [patch 4/8] Types GC [varobj_list to all_root_varobjs]]
- References: <20090525080233.GD13323@host0.dyn.jankratochvil.net> <200907301045.51541.vladimir@codesourcery.com> <20090730075220.GA14542@host0.dyn.jankratochvil.net>
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