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

[Jan Kratochvil <jan.kratochvil@redhat.com>]

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.
------------------------------------------------------------------------------

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.


Thanks,
Jan