Re: [patch 0/4] varobj_list replacement [Re: [patch 4/8] Types GC [varobj_list to all_root_varobjs]]

[Vladimir Prus <vladimir at codesourcery dot com>]

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