GNOME Bugzilla – Bug 416094
Excessive GSList allocation in g_main_context_dispatch
Last modified: 2007-03-15 08:52:04 UTC
Surprisingly one of the most frequent allocators whilst running vte/g-t is the g_slist_prepend() in g_main_context_dispatch. Here we can simply allocate the node on the stack.
Created attachment 84249 [details] [review] Allocate the GSList link on the stack.
Created attachment 84250 [details] [review] Allocate the link on the stack and eliminate the call to g_slist_remove_link() Whilst we are modifying the current->source, we could also insert the sanity check that the local link is the list head and eliminate the call to g_slist_remove_link.
Created attachment 84251 [details] [review] 84250: Allocate the link on the stack and eliminate the call to g_slist_remove_link() [Forget to refresh the patch before sending. ]:
Looks ok to me; it should probably have a comment.
Created attachment 84298 [details] [review] Use a link on the stack, and add a comment justifying why it's safe to do so
Incredibly small nitpick: adding a comment that refers to code that is not there anymore seems a bit confusing to me. If we really want a comment here I think it should explicitly tell how this was done before or just explain the problem in abstract terms. Reading this code feels a bit like: /* No need to use stupid_and_slow_division_by_2 () */ a = a >> 1;
Created attachment 84562 [details] [review] Use a link on the stack, and add a comment justifying why it's safe to do so Okay, Xan I take your point; but AIUI the comment needs to show that the complications of scope have been taken into account and justify why one shouldn't just use the more conventional and more readible g_slist_prepend()/g_slist_remove(). I see it as a plea: /* This is a performance hack - do not revert to g_slist_prepend! */ * considers using that as the comment instead...
looks good to commit
Committed to trunk: r5407 2007-03-15 Chris Wilson <chris@chris-wilson.co.uk> * glib/gmain.c (g_main_dispatch): Replace a g_slist_prepend/g_slist_remove pair with an on-stack link and open coding. (#416094)