IRC on-join sluggishness, profiled

[Ethan Blanton]

Paul Aurich spake unto us the following wisdom:
> I spent a little time this afternoon playing with callgrind and profiling
> the new IRC prpl behavior, in 2.8.0.  (As a side note, anyone know of a
> good replacement for kcachegrind that doesn't require I install a crapton
> of KDE libs?).  This was done profiling joins to #pidgin and #freenode (I
> tried #ubuntu, but gave up after it was still running once I'd finished a
> full episode of The Simpsons)

Awesome!  I looked into this briefly earlier today, but profiling on
OSX is its own special hell.  I'm glad you beat me to it.  ;-)

> I've attached one patch which greatly improves purple_conv_chat_cb_find's
> behavior (it's a significant pig), though we'd need to bump up to 2.9.0
> with this.

This is my biggest problem with the insertion of the problematic code;
I don't see a way to fix it without changing the API.  The API added
for storing attributes on a chatbuddy is terrible, too, but we're
stuck with it until 3.0.0.  The way I see it, we have a couple of
options:

1) Rapidly (like, now) declare the added API in 2.8.0 broken and a
   mistake, and declare that if anyone uses it they get what's coming to
   them.  This isn't *nice*, but it doesn't hang us up until 3.0.0.

2) Keep the bad and broken API as a set of special checks mapped to a
   sane API, which we export in 2.9.0.

This is somewhat orthogonal to your point, which is that the chatbuddy
structure is going to require a collation key.

I'm inclined to suggest going whole hog on this, and making that
collation key a hash key, and implementing hashed chatbuddy searches.
The additional code should be minimal, but it'll hopefully forestall
ever having to visit this issue again.  We did this for the blist
many, many moons ago.  See the _purple_hbuddy stuff.

> The remaining big chunk of time is spent in this call chain:
> 	irc_msg_who
> 		purple_conv_chat_user_set_flags
> 			pidgin_conv_chat_update_user
> 		purple_conv_chat_cb_set_attributes
> 			pidgin_conv_chat_update_user
> 
> This is naturally called once for each user in the WHO, and there are a few
> issues with pidgin_conv_chat_update_user:
> 
> 1) It has an O(n) (n = size of list) search for the current row for the
> chat entry

So ... we also dealt with this for the blist, as well.  GtkTreeViews
have a static reference which allows rapid lookup of a row; looking at
gtkblist.c it looks like it's GtkTreeRowReference.  If we tag one of
these onto ui_data for a chatbuddy, we ought to be able to map to one
immediately.

> 2) The search calls purple_utf8_strcasecmp, which, if I'm reading this
> kcachegrind output right (no guarantee of that), is over half of the cycles
> spent in pidgin_conv_chat_update_user.

If we handle #1, this will go away directly.

> 3) removes and re-adds the row to the list store (this is 11% of the cycles
> for pidgin_conv_chat_update_user)
> 
> 4) Reinserting the row is totally useless for this specific update type.
> 
> I'm not sufficiently familiar with the GTK conversation code and gtk list
> store/tree view code to know if there's a more efficient way to handle #1
> and #3.

#1 can be improved, per above, the question is simply how painful it
is.  I can't answer #3, either.  I don't quote grok your fix for #4,
but I trust you that it's the right solution.  We can even leave the
context-less UI op, and just not use it; that solves backward
compatability.

Ethan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 482 bytes
Desc: Digital signature
URL: <http://pidgin.im/pipermail/devel/attachments/20110619/bd10d26b/attachment.sig>