Created attachment 212064 [details] [review]
application-window: Move desktop file handling code to separate function

Created attachment 212065 [details] [review]
application-window: Try harder to look up the application name

Application names don't always match with .desktop file names, so we
need to do more work to find the correct desktop file.

Review of attachment 212065 [details] [review]:

It would be very easy to get rid of all the #ifdef HAVE_GIO_UNIX code, but that would remove the optimization which currently tries to read in ${executable_name}.desktop and if that fails, falls back to g_app_info_get_all(). I don't really know how to measure g_app_info_get_all() penalty for app startup speed, so I left the HAVE_GIO_UNIX code in for now.

(In reply to comment #0)

> A possible way to fix this would be to read in all desktop files and find which
> one of them references the executable name.

this is not a good plan at all; considering that there is no cache of desktop files to mmap at startup, this would imply hitting the disk multiple times, which will lead to a loss of performance.

what I'm interested in is why Fedora changes the names of desktop files; either they stop doing that, or they submit their patch upstream.

(In reply to comment #3)
> Review of attachment 212065 [details] [review]:
> 
> It would be very easy to get rid of all the #ifdef HAVE_GIO_UNIX code, but that
> would remove the optimization which currently tries to read in
> ${executable_name}.desktop and if that fails, falls back to
> g_app_info_get_all(). I don't really know how to measure g_app_info_get_all()
> penalty for app startup speed, so I left the HAVE_GIO_UNIX code in for now.

it's easy to measure the penaly: just time the startup of an application that matches, and then time the startup of an application that does not match - once without your patch, and once with it. repeat about 10 times to get some stable numbers. blowing the kernel inode caches would also allow you to identify the timing in cold start cases.

Did some testing and with both patches applied, measured the time it takes to execute gtk_application_window_get_app_desktop_name().

hot cache and matching desktop file name (lightsoff.desktop):
0.0003s

hot cache and non-matching desktop file name (gnome-lightsoff.desktop):
0.0311s

cold cache and matching desktop file name (lightsoff.desktop):
0.0022s

cold cache and non-matching desktop file name (gnome-lightsoff.desktop):
1.4789s


I think this shows that it's certainly worth keeping the ${executable_name}.desktop file reading optimization. Where I am unsure is whether it's worth trying to fall back to parsing all the desktop files when:
 - the app doesn't have a name set with g_set_application_name(), and
 - the app doesn't have a matching ${executable_name}.desktop file

--------------------------

Test setup:

11 test runs, dropped first result and calculated the average of the 10 remaining results. For cold cache tests ran "sync && echo 3 > /proc/sys/vm/drop_caches" before doing the test.

The disk used for /usr/share is rotating media.

(In reply to comment #4)

> 
> what I'm interested in is why Fedora changes the names of desktop files; either
> they stop doing that, or they submit their patch upstream.
> 

Mostly historical baggage. The desktop entry spec has this misfeature of 'vendor prefixes', and back in the day, that was thought to be a good idea to apply. Now, desktop file names tend to spread out into all sorts of configuration, so once you've applied such a prefix, you're kinda stuck, unless you are willing to break user configuration.

Review of attachment 212064 [details] [review]:

I think this makes sense, but I have a comment on the code below.

::: gtk/gtkapplicationwindow.c
@@ +322,3 @@
+                  /* get the name from .desktop file */
+                  name = gtk_application_window_get_app_desktop_name ();
+                  if (name == NULL)

You're losing the if (g_strcmp0(app_name, name) == 0) condition here, in which case we still want to show Application

Thanks Cosimoc.

(In reply to comment #7)
> You're losing the if (g_strcmp0(app_name, name) == 0) condition here, in which
> case we still want to show Application

Hm. The way I read this condition is: if the app's executable name matches the name set in .desktop file, show Application. This seems strange; why would we want to show the fallback name (Application) if these two happen to match?

(In reply to comment #8)
 
> Hm. The way I read this condition is: if the app's executable name matches the
> name set in .desktop file, show Application. This seems strange; why would we
> want to show the fallback name (Application) if these two happen to match?

You're right, makes sense; I didn't realize the fallback in GIO for when a missing name is "Unnamed". Feel free to push the patch to the master and gtk-3-4 branches.

(In reply to comment #9)
> (In reply to comment #8)
> 
> > Hm. The way I read this condition is: if the app's executable name matches the
> > name set in .desktop file, show Application. This seems strange; why would we
> > want to show the fallback name (Application) if these two happen to match?
> 
> You're right, makes sense; I didn't realize the fallback in GIO for when a
> missing name is "Unnamed". Feel free to push the patch to the master and
> gtk-3-4 branches.

really? this still means that, with a cold cache, creating a new application will take 1 second more in case the name does not match. I'm not entirely comfortable with a speed regression of this size.

sorry, the first patch looks okay to me; it's the second patch that I don't really like.

Review of attachment 212064 [details] [review]:

Thanks, pushed to both master and gtk-3-4 branches.

(In reply to comment #11)
> sorry, the first patch looks okay to me; it's the second patch that I don't
> really like.

Yes, I think I agree that the second patch is a bad idea.

I'd like to additionally point out that the startup penalty would also happen not only when the names don't match, but also when an app doesn't have a desktop file at all. Which might be even worse.

So the first patch is committed and I'm dropping the second one; I'll take a look if we can rename the desktop files back in Fedora instead.

Thanks everybody!