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 778778 - [PATCH] Optimize startup time by avoiding reading all mime-types from files again
[PATCH] Optimize startup time by avoiding reading all mime-types from files a...
Status: RESOLVED FIXED
Product: rygel
Classification: Applications
Component: MediaExport plugin
git master
Other Linux
: Normal enhancement
: ---
Assigned To: Jens Georg
rygel-maint
Depends on:
Blocks:
 
 
Reported: 2017-02-16 17:24 UTC by Matteo Settenvini
Modified: 2017-02-19 12:53 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (11.53 KB, patch)
2017-02-16 17:24 UTC, Matteo Settenvini
none Details | Review
build fix patch (901 bytes, patch)
2017-02-16 17:24 UTC, Matteo Settenvini
none Details | Review
patch (10.15 KB, patch)
2017-02-16 21:13 UTC, Matteo Settenvini
none Details | Review
patch (10.15 KB, patch)
2017-02-17 13:02 UTC, Matteo Settenvini
none Details | Review
patch (10.56 KB, patch)
2017-02-18 19:02 UTC, Matteo Settenvini
committed Details | Review

Description Matteo Settenvini 2017-02-16 17:24:04 UTC
Created attachment 345971 [details] [review]
patch

Dear maintainer(s),

I am a long-standing Rygel user, and for me it's one of the best solutions for streaming videos in my household with almost no configuration. Thanks for your effort!

However, I have a quite large amount of files which get indexed by the media export plugin (thousands of videos, tens of thousands of music files, tens of thousands of picture files...). All of that is stored on a slow rotational drive, and a cold startup of Rygel can take up to 10 minutes for me, with a lot of disk thrashing.

By using: 
    
    sudo bash -c "echo 3 > /proc/sys/vm/drop_caches"

and strace'ing rygel, I noticed that all files (even if already in the cache) were opened at each startup to determine their mime type.

Here is a patch that always re-uses the mime type from the cache if it is up-to-date (followed by a trivial patch that fixes my build from a clean directory).

This is my first attempt to a patch to Rygel and in Vala, so comments are welcome: I might have gotten something wrong. 

However, with it, I see at least a ten-fold performance improvement on a cold start.

Cheers!
Comment 1 Matteo Settenvini 2017-02-16 17:24:42 UTC
Created attachment 345973 [details] [review]
build fix patch
Comment 2 Jens Georg 2017-02-16 19:29:47 UTC
Review of attachment 345973 [details] [review]:

This will break out of tree build
Comment 3 Jens Georg 2017-02-16 19:34:32 UTC
Review of attachment 345971 [details] [review]:

Hi, thanks for the patch, some minor style adjustments, then it's fine to go in :)

::: src/plugins/media-export/rygel-media-export-harvester.vala
@@ +82,2 @@
         if (is_blacklisted) {
+            debug ("URI %s is not eligble due to blacklisting",

WS-only change?

::: src/plugins/media-export/rygel-media-export-harvesting-task.vala
@@ +152,3 @@
      */
     private bool push_if_changed_or_unknown (File       file,
+                                              FileInfo   info) {

indent wrong

@@ +175,3 @@
             } else {
+                if (info.get_content_type() != "")
+                {

Curly to the previous line, please

@@ +219,2 @@
         if (info.get_file_type () == FileType.DIRECTORY) {
+

What's this change doing?

@@ +325,3 @@
     private void on_extracted_cb (File               file,
                                   Variant?           info) {
+

Same here?
Comment 4 Matteo Settenvini 2017-02-16 20:48:18 UTC
Review of attachment 345971 [details] [review]:

::: src/plugins/media-export/rygel-media-export-harvester.vala
@@ +82,2 @@
         if (is_blacklisted) {
+            debug ("URI %s is not eligble due to blacklisting",

Nope, there was a spelling error ;-)
blacklising vs. blacklisting
Comment 5 Matteo Settenvini 2017-02-16 21:13:41 UTC
Created attachment 345999 [details] [review]
patch

Here we go, I made another small optimization while I was at it.
Comment 6 Matteo Settenvini 2017-02-16 21:53:27 UTC
(In reply to Jens Georg from comment #2)
> Review of attachment 345973 [details] [review] [review]:
> 
> This will break out of tree build

For me it does build only in-tree WITHOUT this patch... example:

make[3]: Entering directory '/home/matteo/.cache/gnome-builder/builds/Rygel/local/x86_64-linux-gnu/data/xml'
 cd /home/matteo/Progetti/rygel && /bin/bash /home/matteo/Progetti/rygel/build-aux/missing automake-1.15 --gnu data/xml/Makefile
 cd ../.. && /bin/bash ./config.status data/xml/Makefile 
config.status: creating data/xml/Makefile
make[3]: *** No rule to make target 'MediaServer3.xml', needed by 'all-am'.  Stop.
make[3]: Leaving directory '/home/matteo/.cache/gnome-builder/builds/Rygel/local/x86_64-linux-gnu/data/xml'
Makefile:775: recipe for target 'all-recursive' failed
make[2]: *** [all-recursive] Error 1
make[2]: Leaving directory '/home/matteo/.cache/gnome-builder/builds/Rygel/local/x86_64-linux-gnu/data'
Makefile:596: recipe for target 'all-recursive' failed
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory '/home/matteo/.cache/gnome-builder/builds/Rygel/local/x86_64-linux-gnu'
Makefile:497: recipe for target 'all' failed
make: *** [all] Error 2


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

matteo@settenvini:~/.cache/gnome-builder/builds/Rygel/local/x86_64-linux-gnu/data/xml$ make --debug=i
GNU Make 4.1
Built for x86_64-pc-linux-gnu
Copyright (C) 1988-2014 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Reading makefiles...
Updating goal targets....
 File 'all' does not exist.
   File 'all-am' does not exist.
     Looking for an implicit rule for 'EnergyManagement.xml'.
     Trying pattern rule with stem 'EnergyManagement'.
     Trying implicit prerequisite '/home/matteo/Progetti/rygel/data/xml/EnergyManagement.xml.in'.
     Found an implicit rule for 'EnergyManagement.xml'.
       Looking for an implicit rule for '/home/matteo/Progetti/rygel/data/xml/EnergyManagement.xml.in'.
       Trying pattern rule with stem 'EnergyManagement.xml.in'.
       Trying implicit prerequisite '/home/matteo/Progetti/rygel/data/xml/EnergyManagement.xml.in,v'.
       Trying pattern rule with stem 'EnergyManagement.xml.in'.
       Trying implicit prerequisite '/home/matteo/Progetti/rygel/data/xml/RCS/EnergyManagement.xml.in,v'.
       Trying pattern rule with stem 'EnergyManagement.xml.in'.
       Trying implicit prerequisite '/home/matteo/Progetti/rygel/data/xml/RCS/EnergyManagement.xml.in'.
       Trying pattern rule with stem 'EnergyManagement.xml.in'.
       Trying implicit prerequisite '/home/matteo/Progetti/rygel/data/xml/s.EnergyManagement.xml.in'.
       Trying pattern rule with stem 'EnergyManagement.xml.in'.
       Trying implicit prerequisite '/home/matteo/Progetti/rygel/data/xml/SCCS/s.EnergyManagement.xml.in'.
       No implicit rule found for '/home/matteo/Progetti/rygel/data/xml/EnergyManagement.xml.in'.
     File 'MediaServer3.xml' does not exist.
     Looking for an implicit rule for 'MediaServer3.xml'.
     Trying pattern rule with stem 'MediaServer3'.
     Trying implicit prerequisite '/home/matteo/Progetti/rygel/data/xml/MediaServer3.xml.in'.
     Trying pattern rule with stem 'MediaServer3.xml'.
     Trying implicit prerequisite 'MediaServer3.xml,v'.
     Trying pattern rule with stem 'MediaServer3.xml'.
     Trying implicit prerequisite 'RCS/MediaServer3.xml,v'.
     Trying pattern rule with stem 'MediaServer3.xml'.
     Trying implicit prerequisite 'RCS/MediaServer3.xml'.
     Trying pattern rule with stem 'MediaServer3.xml'.
     Trying implicit prerequisite 's.MediaServer3.xml'.
     Trying pattern rule with stem 'MediaServer3.xml'.
     Trying implicit prerequisite 'SCCS/s.MediaServer3.xml'.
     Trying pattern rule with stem 'MediaServer3'.
     Trying implicit prerequisite '/home/matteo/Progetti/rygel/data/xml/MediaServer3.xml.in'.
     Looking for a rule with intermediate file '/home/matteo/Progetti/rygel/data/xml/MediaServer3.xml.in'.
      Avoiding implicit rule recursion.
      Trying pattern rule with stem 'MediaServer3.xml.in'.
      Trying implicit prerequisite '/home/matteo/Progetti/rygel/data/xml/MediaServer3.xml.in,v'.
      Trying pattern rule with stem 'MediaServer3.xml.in'.
      Trying implicit prerequisite '/home/matteo/Progetti/rygel/data/xml/RCS/MediaServer3.xml.in,v'.
      Trying pattern rule with stem 'MediaServer3.xml.in'.
      Trying implicit prerequisite '/home/matteo/Progetti/rygel/data/xml/RCS/MediaServer3.xml.in'.
      Trying pattern rule with stem 'MediaServer3.xml.in'.
      Trying implicit prerequisite '/home/matteo/Progetti/rygel/data/xml/s.MediaServer3.xml.in'.
      Trying pattern rule with stem 'MediaServer3.xml.in'.
      Trying implicit prerequisite '/home/matteo/Progetti/rygel/data/xml/SCCS/s.MediaServer3.xml.in'.
     No implicit rule found for 'MediaServer3.xml'.
    Must remake target 'MediaServer3.xml'.
make: *** No rule to make target 'MediaServer3.xml', needed by 'all-am'.  Stop.
Comment 7 Jens Georg 2017-02-17 07:45:40 UTC
(In reply to Matteo Settenvini from comment #6)
> (In reply to Jens Georg from comment #2)
> > Review of attachment 345973 [details] [review] [review] [review]:
> > 
> > This will break out of tree build
> 
> For me it does build only in-tree WITHOUT this patch... example:
> 
> make[3]: Entering directory
> '/home/matteo/.cache/gnome-builder/builds/Rygel/local/x86_64-linux-gnu/data/
> xml'

Ah, nasty. I usually don't do out-of-tree builds from git since vala will (or used to) put the C files into the source dir anyway. Can you make this a separate ticket, please?
Comment 8 Jens Georg 2017-02-17 08:03:51 UTC
Review of attachment 345999 [details] [review]:

Minor style issues, otherwise good to go

::: src/plugins/media-export/rygel-media-export-harvesting-task.vala
@@ +173,3 @@
                 }
             } else {
+                if (info.get_content_type() != "") {

Space before (

@@ +177,3 @@
+                                            (HARVESTER_MIME_TYPE_ATTRIBUTES,
+                                             FileQueryInfoFlags.NONE);
+                    info.set_content_type(extended_info.get_content_type());

Minor nitpick: Space before (
Comment 9 Matteo Settenvini 2017-02-17 13:02:22 UTC
Created attachment 346068 [details] [review]
patch

Whitespace fixes. Obsoleting also build fix patch, moving to a separate bug.
Comment 10 Jens Georg 2017-02-18 09:17:22 UTC
THere's an issue there: without the patch, I get
rygel-media-export-metadata-extractor.vala:268: Sent command to extractor process: EXTRACT file:///home/jens/Pictures/2013/05/25/DSCF0524.RAF|image/x-fuji-raf

With the patch:

rygel-media-export-metadata-extractor.vala:268: Sent command to extractor process: EXTRACT file:///home/jens/Pictures/2013/05/25/DSCF0524.RAF|directory/inode

Which breaks the blacklisting
Comment 11 Jens Georg 2017-02-18 09:27:13 UTC
> Which breaks the blacklisting

Ah. No it doesn't but the rest still applies
Comment 12 Jens Georg 2017-02-18 09:54:34 UTC
But it seems to break the image fast path for the initial crawl
Comment 13 Matteo Settenvini 2017-02-18 19:02:49 UTC
Created attachment 346146 [details] [review]
patch

Thanks for catching that; it seems I was overzealous with the previous refactoring, and was querying the mime_type of the origin folder instead of the current file. This should fix it.
Comment 14 Jens Georg 2017-02-19 12:52:55 UTC
Thanks!

Before:
MediaExport-Message: rygel-media-export-harvesting-task.vala:300: Harvesting
of file:///home/jens/Videos done in 0,604746
MediaExport-Message: “file:///home/jens/Music” harvested
MediaExport-Message: rygel-media-export-harvesting-task.vala:300: Harvesting
of file:///home/jens/Music done in 1,677375
MediaExport-Message: “file:///home/jens/Pictures” harvested
MediaExport-Message: rygel-media-export-harvesting-task.vala:300: Harvesting
of file:///home/jens/Pictures done in 2,103084

After:
MediaExport-Message: rygel-media-export-harvesting-task.vala:309: Harvesting
of file:///home/jens/Videos done in 0,273338
MediaExport-Message: “file:///home/jens/Music” harvested
MediaExport-Message: rygel-media-export-harvesting-task.vala:309: Harvesting
of file:///home/jens/Music done in 1,006804
MediaExport-Message: “file:///home/jens/Pictures” harvested
MediaExport-Message: rygel-media-export-harvesting-task.vala:309: Harvesting
of file:///home/jens/Pictures done in 1,398075