After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 767765 - Add names and tags to various GSources and GTasks constructed in GLib
Add names and tags to various GSources and GTasks constructed in GLib
Status: RESOLVED OBSOLETE
Product: glib
Classification: Platform
Component: mainloop
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on: 447907
Blocks:
 
 
Reported: 2016-06-16 23:01 UTC by Philip Withnall
Modified: 2018-05-24 18:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gmain: Add G_PID_FORMAT (2.77 KB, patch)
2016-06-16 23:01 UTC, Philip Withnall
committed Details | Review
gmain: Add names to various GSources constructed in GLib (7.63 KB, patch)
2016-06-16 23:01 UTC, Philip Withnall
none Details | Review
gio: Add source tags to various GTasks constructed in GLib (41.45 KB, patch)
2016-06-16 23:40 UTC, Philip Withnall
committed Details | Review
gmain: Add names to various GSources constructed in GLib (4.51 KB, patch)
2018-02-16 12:20 UTC, Philip Withnall
none Details | Review

Description Philip Withnall 2016-06-16 23:01:13 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.
Comment 1 Philip Withnall 2016-06-16 23:01:16 UTC
Created attachment 329919 [details] [review]
gmain: Add G_PID_FORMAT

This will be useful for printing out GPids in printf()-style functions.
Comment 2 Philip Withnall 2016-06-16 23:01:19 UTC
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.
Comment 3 Philip Withnall 2016-06-16 23:02:09 UTC
The changes in GIO depend on g_flags_to_string() from bug #447907.
Comment 4 Philip Withnall 2016-06-16 23:40:36 UTC
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.
Comment 5 Matthias Clasen 2016-06-20 12:27:22 UTC
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.
Comment 6 Matthias Clasen 2016-06-20 12:27:57 UTC
Review of attachment 329923 [details] [review]:

Looks good
Comment 7 Matthias Clasen 2016-06-20 12:34:25 UTC
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

?
Comment 8 Philip Withnall 2016-06-29 14:21:57 UTC
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
Comment 9 Philip Withnall 2016-06-29 14:29:53 UTC
(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.
Comment 10 Matthias Clasen 2016-06-29 17:40:33 UTC
(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.
Comment 11 Philip Withnall 2018-02-16 12:20:24 UTC
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.
Comment 12 Philip Withnall 2018-02-16 12:21:14 UTC
Updated the patch to drop all the allocations. Matthias?
Comment 13 GNOME Infrastructure Team 2018-05-24 18:57:42 UTC
-- 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.