patch

Patch and changelog (for libbonoboui).

After exploring many debugging outputs and gdb stack traces I realized that an accelerator was changed whenever the node of the xml tree that represented a specific menu widget was marked as “dirty”. In the process of merging the several xml trees a node could be set dirty depending on the command that represent. Some commands need to be synchronized; this sync is transferred to the node, for example in impl_bonobo_ui_sync_menu_state. But the problem that this function had was the chaotic addition of accelerators which made an expensive growth of structures, memory leaks included.

The patch respects the fact that we can have a widget with multiple accelerators and that the same accelerator can be added many times in the same accelerator group. The code only avoid the accelerator addition if it has been added previously to the same widget (which is the common case).

A chart with the difference of the old and new behaviour of component mail switching (200 switches).

200 switches

After each switch, evo sleep, for avoiding the linux scheduler interference. Other charts didn’t take into account that, but it doesn’t matter, because the effects and conclusions were the same. For example, the following chart represents a massive 200-switching. The old version never stops growing and the new one become stable in a specific threshold, determined by the end of an epoch in the Linux scheduler (probably evo should be at the end of the runqueue, but starvation doesn’t happen).

200 switches

* Edited * I’ve changed the patch because it had a little problem of memory leaking. Thanks Benoît Dejean for your remark!

About these ads

15 Comments »

  1. Alex Jones said

    Good work, Cecilia! :)

  2. Emmanuele said

    great work indeed; am I wrong or this should also improve performances for panel applets and other bonobo users out there?

  3. Raphael Bosshard said

    Woot! Nice Work!

  4. andre said

    this is great, so much love has been spent on evolution for the last 6 months, especially with regard to speed improvements and memory fixes.

    so a big thanks to cecilia, and to everybody in the gnome community!

  5. Andreas said

    Wow! Great job!

  6. Kurt McKee said

    Excellent work! Thank you!

  7. Ken said

    Excellent…

    This is only a fix for Evo, right? Setting accelerators in Gtk+ is still (relatively) slow, it sounds like.

  8. Matthew W. S. Bell said

    Nice.

  9. Roberto said

    like u, contact me.

  10. Benoît Dejean said

    isn’t your patch leaking ?

    + if (n_entries)
    + for (l_tmp = l_closures; l_tmp; l_tmp = l_tmp->next)
    + for (i=0; idata)
    + return TRUE;
    +
    + g_list_free (l_closures);
    +
    + return FALSE;
    +}

    you forgot to free l_closures, didn’t you ?

  11. pedro said

    Amazing work, Cecilia. Congratulations!

  12. Lee Willis said

    Wouldn’t this be better going into GTK itself (Therefore fixing it for any other cases where this could occur) as well?

    I can’t think of any reason why it would ever be valid to add the same accelerator to a widget multiple times?

    Was there a particular reason this got fixed in bonobo, and not lower down in GTK?

  13. m3gumi said

    Thank you all for your comments!

    I hope this patch improve performace on applications using bonoboui.

    Benoît Dejean: I free l_closures at the end of the function.

    Lee Willis: I found the patch easier to implement in bonobo and I didn’t know if I should forbid the addition of the same accelerator in GTK, though it looks odd, I know.

  14. Benoît Dejean said

    > Benoît Dejean: I free l_closures at the end of the function.

    Not if you return TRUE;

  15. m3gumi said

    Benoît Dejean:
    Of course, you’re right! Thanks, I’ll correct my stupid mistake.

RSS feed for comments on this post · TrackBack URI

Leave a Reply

Please log in using one of these methods to post your comment:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

Follow

Get every new post delivered to your Inbox.

%d bloggers like this: