Re: bonobo-ui-node re-write ...

[Michael Meeks <michael ximian com>]

Hi Darin,

	Thanks for your comments and eyes on the code,

On Wed, 22 Aug 2001, Darin Adler wrote:
> Did you do any timings to see if it's faster, or are you expecting
> other people to do that? (Nice luxury if you can get it.)

	Since the re-write was primarily not intended to speed it up, I
didn't do any hard bench marking, no. One might imagine that it might as a
side effect make things faster, simple because of less malloc thrash, and
locking in threaded apps; and becuase of faster compares.

	I did note that it reduced the memory consumption of the (somewhat
small UI ) in the test-ui app by ~100Kb though, or at least so it seemed
from a quick RSS - SHARE top session.

> It seems that you've reinvented GQuark here. You should use GQuarks
> for the IDs, rather than having the additional complexity of your own
> hash tables.

	I'm well aware of GQuark; GQuark's reverse mapping id -> string is
linear in the number of strings, bonobo-ui-node's is constant time. Since
we do rather a number of id -> string mappings as we output xml it seemed
good to me to do that.

> This even allows a bit of additional optimization by using
> g_quark_from_static_string when appropriate.

	Well - since we free the strings at exit ( a practice of dubious
merit it seems ), this would cost more by needing to flag static strings
separately.

> Overall, the change seems like a great idea. It does seem gratuitous
> to make the code rely on the BonoboUINode structure rather than
> continuing to use function calls to get things like next, previous,
> and parent. If that change results in a measurable speedup, then I
> guess it's worth it, but I doubt it does. Keeping the abstraction if
> it's not prohibitively expensive might make it easier to make a future
> change in this area.

	Gnome 1.4 is end of development lifetime and well into maintenance
mode I hope, and I cannot envisage us switching to another UI node
structure any time soon - so this doesn't worry me. But I agree, I only
did the switch where I was supicious that I'd seen the method on a profile
in the past ( not a good optimization heuristic though ).

> > +	if (!(txt = bonobo_ui_node_get_attr_by_id (node, id_id))) {
> >
> > +		txt = bonobo_ui_node_get_attr_by_id (node, verb_id);
> >
> > +		if (txt && txt [0] == '\0')
> > +			txt = bonobo_ui_node_get_attr_by_id (node, name_id);
> > +	}
>
> Looks to me like the above code is missing a ! in the if statement.

	I don't follow you - there was no functional change here. It is to
allow verb="" to fallback to the node name ( AFAICS ).

> I think there's a faster way to check for non-zero than calling atoi.
> But perhaps this is not a bottleneck.

	:-) doesn't show on my profiles - it's all redundant pixmap
scaling, which I'm just looking at since I'm here; again no code change
here.

> You should put a (void) in there, instead of ().

	Yes; if fixed too.

> You don't need the ? : here. All strdup functions (g_strdup,
> xmlStrdup) in GNOME are designed to work on NULL too. So you can just
> do:

	Fixed.

> You don't need the ? : here. All strdup functions (g_strdup,
> xmlStrdup) in GNOME are designed to work on NULL too. So you can just
> do:

	Why cut and paste the same message ? as soon as I realized this I
fixed it everywhere - naturaly.

> > +static void
> > +uiCharacters (ParseState *ps, const xmlChar *chars, int len)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < len; i++)
> > +	    g_string_append_c (ps->content, chars [i]);
> > +}

	Did you mean to comment on the above ?

> Using
>
> > +	g_return_val_if_fail (xml != NULL, NULL);
> >
> > +	len = strlen (xml);
> > +	if (len < 4)
> >  		return NULL;
>
>
> Why a warning for NULL, a warning for XML that's not well formed, but
> no warning for a too-short string? You should warn here too, I think.
> Is there a reason you need this check? Won't xmlSAXUserParseMemory
> detect the error? Is there a reason to optimize the case of passing in
> a bad string?  Does that happen enough to affect speed?

	Several reasons; firstly it is reasonable to pass "" to this
method, and it is fair to get NULL back as the XML representation of an
empty string I feel.

	Secondly, if you examine the memory parser you see our string soon
hits:

	xmlCharEncoding
	xmlDetectCharEncoding(const unsigned char* in)
	{
	    if ((in[0] == 0x00) && (in[1] == 0x00) &&
	        (in[2] == 0x00) && (in[3] == 0x3C))
		return(XML_CHAR_ENCODING_UCS4BE);

	So it seems good to me to protect the user from the potential of a
hidden seg fault as it runs over the end of the string - a little known
libxml feature.

> > +	contains = node->content || (node->children && recurse);
>
> Why put this in a local variable? You should just put this test in the
> if statement below.

	Indeed, originaly I used this twice hence the variable, but since
refactoring I don't. The compiler will optimize it away nicely - it seems
reasonable to leave it since it makes the method more readable.

	Any more queries ? or better still runtime issues ...

	Regards,

		Michael.

-- 
 mmeeks gnu org  <><, Pseudo Engineer, itinerant idiot