Created attachment 68877 [details]
valgrind start up with plugins enabled

Profile of RB startup and then exit from menu.  Most plugins are enabled.

Created attachment 68878 [details]
valgrind start up with all plugins disabled

Same but with all plugins disabled.

Created attachment 69465 [details] [review]
use RBRefString for LOCATION property

One thing that might help is using RBRefStrings for the entry URIs. It does mean that there will be an additional integer and two pointers for every entry in the DB, however this is offset by a few things:

During startup the db g_strdup()s the URIs for several things, which a g_free()d a short time later. Using RBRefString stop the allocation/deallocation which reduces heap-fragmentation. Although the amount of memory "in use" is slightly higher, the actual size of the heap seems (for me) to be slightly smaller due to a less fragmented heap.

RBStaticPlaylistSource holds a g_strdup()d copy of the URI of each entry in it. Which means we have an extra URI for each time the entry is in a static playlist. Using RBRefString would allow us to just ref it, instead of copying. For me this saves ~500k of space, and potentially more for someone who heavily uses static playlists.


This patch makes RhythmDBEntry use RBRefStrings for the location and playback-error properties, refs the instead of strdup()ing thoughout the rhtyhmdb and static playlist code.

RBRefString also has an advantage when used in hash tables/maps, it only requires a strcmp when created, not whenever it is looked up.

Looks like a good idea to me.

Created attachment 69591 [details] [review]
resync with CVS

Resync with CVS after the leak fix changes.

This looks good to me as a first step.

The next step I think is to break up the URI into parts and store those refstrings separately.  Most of the URIs will have the exact same prefix if they are in the "Library".  We should be able to store that once.  Then, most of the time, each track in an album has the same album name in the URI too.  Thought that is a smaller effect.

I think it would be hard to add something like this internally to RefEntry since the segmentation of the string would be difficult to do generally.  On the other hand it is pretty easy for URIs since the separator is well known and there are no translation issues.

We might be able to change the location field to be a GPtrArray of RefStrings and change the db->priv->entries hash back to a string hash of uris.

Created attachment 69675 [details] [review]
experiment with refwords

On the other hand.  One of the potential problems with using consts is that it requires that the strings (all strings) stay loaded at all times.  Here's an experiment that changes refstrings to optionally be segmented into "words".  The idea would be to only create the strings on demand.  To use this most effectively we'd have to modify all the other data models to store the refstring directly instead of a copy of the generated string.

I wonder if this scales better this way.  With the library I have on my development system (~1000 songs) the memory usage with this patch is about the same as HEAD maybe a little higher - probably due to fragmentation or some leaks introduced by un-consting.  Anyway food for thought.

I've committed my patch to cvs.



I'd though about something like that before, but in a naive implementation the overhead is probably too great. A RefString will need a pointer to each of the RefWords, an int for the number of RefWords (or a pointer to be NULL terminated) and the overhead of malloc()ing the GPtrArray block. On a 32 bit system that adds up to almost as much as we save by not duplicating data, and on a 64 bit system (where pointers are twice as big) it's usually much worse.


I had an idea last night, on a potentially smarted way to do this for URIs. We would make the location be split into exactly two RefStrings, where the first is always a valid prefix listed by the entry-type or NULL.

When adding a new entry we see if the URI starts with any of the valid prefixes, and split it there if it is, or set the prefix to NULL and the suffix to the entire URI.

For the SONG entry-type, the prefixes would be the user-set library location(s), as that's where most people keep their music. For removable media, it would be the mount point.

This would probably save a fair bit of memory because most entries would have a valid prefix. There wouldn't be that much overhead because we can just have two RBRefString pointers in RhythmDBEntry, and we don't have to malloc() a GPtrArray for it.



In any case, I think getting rid of the const char* use would be helpful, and make everything use either char* or RBRefString*. I guess a first pass would be to get rid of rhythmdb_entry_get_string and replace it with either _dup_string or _get_refstring.

There is a problem with this commit:

backtrace of rhythmbox --g-fatal-warnings
RhythmDB-WARNING **: attempting to create entry that already exists: \xa4\u0001

gdb> bt full

+ Trace 69680

Without --g-fatal-warnings (and within valgrind, I can't run it because glibc detected, invalid free)

(rhythmbox:23735): RhythmDB-CRITICAL **: rb_refstring_unref: assertion `val->refcount > 0' failed

(rhythmbox:23735): RhythmDB-CRITICAL **: rb_refstring_unref: assertion `val->refcount > 0' failed
RhythmDB-ERROR **: file rhythmdb.c: line 1978 (queue_stat_uri): assertion failed: (uri && *uri)
aborting...

Hmm.. I somehow got entries with empty locations :(

I've committed a fix to cvs that might (hopefully) fix the crash. I missed a g_free->rb_refstring_unref change.

Created attachment 69802 [details] [review]
Use refstrings for DAAP playlists too

Similar to the change for static playlists, this uses RBRefString for daap playlists URIs.

I've committed the daap playlist patch.

I've taken some measurements of RB's heap usage with different things used:


* Baseline (no plugins, empty library): 5Mb
About half of this seems to be Pango/Freetype related.

* 5000 song library (no plugins, no playlists): 8Mb
About 1Mb of refstrings (most of which are URIs), 750k of RhythmDBEntry structures, and assorted other stuff (more Pango/FT, maps for the query models etc).

* 5000 song library, 5 large auto playlists (no plugins): 10Mb
Most of the extra space is taken up for the query models' forward (GSequence) and reverse (GHashTable) maps, and a lesser amount by the RhythmDBPropertyModel's entry-present table.

* 5000 song library, 1 static playlist containing all 5k tracks (no plugins): 9.5Mb:
As above, most of the extra space was taken up by the query model's per-entry data, but RBPlaylistSource's entry-map added data too.

* 5000 song library (All C plugins, no playlists): ~8Mb
Turning on all the C plugins makes basically no difference

* 5000 song library (Sample Python plugins, no playlists): 11Mb
Loading Python and PyGTK is fairly expensive.

* 5000 song library (All Python plugins, no playlists): ~11Mb
Turning on extra Python plugins increase heap size significantly.



Results:

* Activating python plugins support is expensive; but once you have one loaded, extra python plugins don't make much of a difference.

From what I've heard PyGTK loads *everything* when you use any of it, which uses a fair bit of memory, and the PyGTK people are currently working on this. I'm not sure how much that will reduce it though.


* The data structures RhythmDBQueryModel uses take up a fair bit of space, however they are used because they reduce a large number of operations' running times (e.g. logN instead of N, NlogN instead of N^2, etc).

It would be *great* if we could find some structure that used less memory without making the asymoptotic running time worse. But it would also be good if we looked at how they are used, to see whether the tradeoff is actually worth it - increasing from logN to N may not make much of a difference (but it might).


* How useful is RBPlaylistSource's entry map? could we replace the "is in map" operation by "rhythmdb_query_model_entry_to_iter (rhythmdb_lookup_by_location())" ?


* Does RhythmDBPropertyModel really need a hash table of all the entries it is counting? The information is necessary because otherwise it doesn't know what it  has counted, but there might be a smarter way of storing it.

RBPlaylistSource's entry map is used to handle entries that don't yet exist - if the entry location is in the map when the entry is created, the entry gets added to the playlist.  We could use a separate map for pending entries instead, or just a list.  Using rhythmdb_query_model_entry_to_iter would require some reworking of the playlist add/remove functions, but that's probably a good thing.

Measurements with the DAAP server enabled, both with and without playlists, would also be interesting.

Created attachment 72160 [details] [review]
reduce property model memory usage

This changes the property model entries hash table to only store hidden entries.  I've tested it with hidden<->visible changes and property value changes, and it seems to work correctly, but more testing would be good.

This reduces heap usage by about 2M for me (somewhere around 15%).

Looks okay to me, and reduces my heap usage a reasonable amount too.

OK, committed.

Would it be possible to get rid of the reverse_map's by simply storing an EggSequenceIter in RhythmDBEntry?

(FWIW; I don't know the rhythmbox code at all)

That wouldn't work, as an entry (i.e. a track) can be in multiple query models.

If the query model is sorted (which all except the ones for static playlists are), we could use egg_sequence_search to do the reverse lookup. The important question is whether the log(n) time version the current O(1) time will make any noticable difference.

Maybe each entry could store a GPtrArray of all the EggSequenceIters it has and then use egg_sequence_iter_get_sequence() to find out which of the models the iter corresponds to? Assuming the number of models is small, a linear search in such an array should not be too bad. This would also be O(log n) since egg_sequence_iter_get_sequence() is O(log n).

I don't know if that O(log n) would make a noticable difference vs. current O(1), but the treap based GSequence in current glib trunk is quite a bit faster than the EggSequence in rhythmbox, in particular for things like get_sequence() which could be rather slow with the splay tree.

Created attachment 82783 [details] [review]
remove reverse_map

(In reply to comment #21)
> Maybe each entry could store a GPtrArray of all the EggSequenceIters it has and
> then use egg_sequence_iter_get_sequence() to find out which of the models the
> iter corresponds to? Assuming the number of models is small, a linear search in
> such an array should not be too bad. This would also be O(log n) since
> egg_sequence_iter_get_sequence() is O(log n).

That could work, although query-models sit at the layer above the core database and entries - so we'd be pushing upper layer stuff down.


> I don't know if that O(log n) would make a noticable difference vs. current
> O(1), but the treap based GSequence in current glib trunk is quite a bit faster
> than the EggSequence in rhythmbox, in particular for things like get_sequence()
> which could be rather slow with the splay tree.

The attached patch replaces the hash-table use with a sequence search, for query models which are sorted.

For me (~6000 songs, ~10 large auto playlists, optimisation disabled) this make Rhythmbox use around 5% more CPU-time on startup, for a 400-500 Kb reduction in memory use. With an optimised build


The extra CPU time doesn't really cause a noticable delay, since most of it is processing my auto playlists, which I'm not looking at when Rhythmbox starts. Query model operations apart from the initial "build everything" at startup are pretty short, and the speed difference is lost in the noise.

I'll have to look into glib's GSequence to see whether the treap makes a difference.

Created attachment 85020 [details] [review]
updated 'remove reverse map'

Using GSequence, this seems to make my startup about 0.1 seconds slower and reduces the memory usage by a few hundred kb.

Created attachment 99604 [details] [review]
Reduce definitely lost leakage without plugins

This patch reduces "definitely lost" memory from "92,109 bytes in 344 blocks" to "81,966 bytes in 169 blocks".  This is with a small 41 song library, and also a no plugins enabled.  I used the following valgrind configurations:
G_SLICE=always-malloc G_DEBUG=gc-friendly valgrind --tool=memcheck --leak-check=full --leak-resolution=high --show-reachable=yes /usr/local/bin/rhythmbox

Committed to svn, with a couple of others I had found locally.

I've reverted this change:

===================================================================
--- shell/rb-tray-icon.c	(revision 5455)
+++ shell/rb-tray-icon.c	(working copy)
@@ -218,6 +218,7 @@
 	g_object_ref (icon->priv->playing_image);
 	g_object_ref (icon->priv->not_playing_image);
 
+	gtk_container_add (GTK_CONTAINER (icon->priv->ebox), icon->priv->playing_image);
 	gtk_container_add (GTK_CONTAINER (icon->priv->ebox), icon->priv->not_playing_image);
 
 	gtk_container_add (GTK_CONTAINER (icon), icon->priv->ebox);

because it causes gtk warnings on startup.  John, if this change was intentional, could you explain why it's necessary?

This bug has gotten a bit general and hasn't seen action in a while.  Going to mark it fixed for now.  Specific issues can be addressed in new bugs.