GNOME Bugzilla – Bug 767765
Add names and tags to various GSources and GTasks constructed in GLib
Last modified: 2018-05-24 18:57:42 UTC
Adding names makes them easier to identify when profiling and debugging; if we set a default name it can (and should) always be overridden by the caller with something more specific. But having a default is good for the cases where that does not happen.
Created attachment 329919 [details] [review] gmain: Add G_PID_FORMAT This will be useful for printing out GPids in printf()-style functions.
Created attachment 329920 [details] [review] gmain: Add names to various GSources constructed in GLib For the purposes of debugging, it is quite useful for every GSource to have a name set. Ensure that any GSource we construct inside GLib has a name set. For GSources which are then returned to the caller, this name can then be overridden with something even more useful by the caller.
The changes in GIO depend on g_flags_to_string() from bug #447907.
Created attachment 329923 [details] [review] gio: Add source tags to various GTasks constructed in GLib This makes them easier to identify when debugging and profiling. This patch was somewhat less than interesting to write.
Review of attachment 329920 [details] [review]: I see that it is tempting to put more information in the name, but this is causing us to do quite a bit of string formatting in potentially hot code paths - maybe we should stick to fixed strings as names ? ::: glib/gmain.c @@ +4695,3 @@ + name = g_strdup_printf ("%ums timeout", interval); + g_source_set_name (source, name); + g_free (name); Not sure about the usefulness of this - the interval is readily available in the struct anyway. The name is really only helpful if it indicates the origin of the source. Which this doesn't.
Review of attachment 329923 [details] [review]: Looks good
Review of attachment 329919 [details] [review]: ::: configure.ac @@ +2888,2 @@ typedef $g_pid_type GPid; +#define G_PID_FORMAT $g_pollfd_format Typo ? Should this be #define G_PID_FORMAT $g_pid_format ?
Pushing the accepted patches, with a fixup for the copypasta problem in the G_PID_FORMAT patch. Attachment 329919 [details] pushed as 7ea4949 - gmain: Add G_PID_FORMAT Attachment 329923 [details] pushed as 3613b7a - gio: Add source tags to various GTasks constructed in GLib
(In reply to Matthias Clasen from comment #5) > Review of attachment 329920 [details] [review] [review]: > > I see that it is tempting to put more information in the name, but this is > causing us to do quite a bit of string formatting in potentially hot code > paths - maybe we should stick to fixed strings as names ? I would lean towards providing more information, given that creating and attaching a GSource is not a fast operation (various allocations for the GSource, and various locking in gmain.c). If g_source_new() is being called on a hot path, I think the application has bigger things to worry about than some printf() calls. > ::: glib/gmain.c > @@ +4695,3 @@ > + name = g_strdup_printf ("%ums timeout", interval); > + g_source_set_name (source, name); > + g_free (name); > > Not sure about the usefulness of this - the interval is readily available in > the struct anyway. The name is really only helpful if it indicates the > origin of the source. Which this doesn't. It’s not possible to indicate the origin of the source without unwinding the stack (let’s not do that) or setting the name in the calling code (which is what we hope will happen anyway). The timeout period is fairly useful for disambiguating timeouts (since they do often differ), and I think there’s no harm in keeping it if we’re keeping the printf()s. If we’re not keeping the printf()s, obviously the timeout length goes.
(In reply to Philip Withnall from comment #9) > (In reply to Matthias Clasen from comment #5) > > Review of attachment 329920 [details] [review] [review] [review]: > > > > I see that it is tempting to put more information in the name, but this is > > causing us to do quite a bit of string formatting in potentially hot code > > paths - maybe we should stick to fixed strings as names ? > > I would lean towards providing more information, given that creating and > attaching a GSource is not a fast operation (various allocations for the > GSource, and various locking in gmain.c). If g_source_new() is being called > on a hot path, I think the application has bigger things to worry about than > some printf() calls. Here is some random selection from GTK+: "[gtk+] drag_scan_timeout" "[gtk+] gtk_text_view_selection_bubble_popup_cb" "[gtk+] slide_idle_handler" "[gtk+] tooltip_browse_mode_expired" "[gtk+] tooltip_popup_timeout" "[gtk+] auto_expand_timeout" "[gtk+] validate_rows" "[gtk+] scroll_sync_handler" "[gtk+] scroll_row_timeout" "[gtk+] open_row_timeout" "[gtk+] gtk_tree_view_search_entry_flush_timeout" "[gtk+] gtk_tree_view_real_search_enable_popdown" "[gtk+] gtk_tree_view_search_entry_flush_timeout" As you see, we got by just fine without printfs. Anyway, if you get Alison to vote for printfs, I'll back down. For now, I'll let my old-school allocation aversion show, and say: no printfs. > > ::: glib/gmain.c > > @@ +4695,3 @@ > > + name = g_strdup_printf ("%ums timeout", interval); > > + g_source_set_name (source, name); > > + g_free (name); > > > > Not sure about the usefulness of this - the interval is readily available in > > the struct anyway. The name is really only helpful if it indicates the > > origin of the source. Which this doesn't. > > It’s not possible to indicate the origin of the source without unwinding the > stack (let’s not do that) or setting the name in the calling code (which is > what we hope will happen anyway). The timeout period is fairly useful for > disambiguating timeouts (since they do often differ), and I think there’s no > harm in keeping it if we’re keeping the printf()s. If you want my opinion, it is just not useful enough.
Created attachment 368417 [details] [review] gmain: Add names to various GSources constructed in GLib For the purposes of debugging, it is quite useful for every GSource to have a name set. Ensure that any GSource we construct inside GLib has a name set. For GSources which are then returned to the caller, this name can then be overridden with something even more useful by the caller. Since this data is only used for debugging, avoid doing any allocations for it; just use static strings.
Updated the patch to drop all the allocations. Matthias?
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/1175.