One thing I didn't point out enough in the last post, was that there is no way we'd commit that stuff as-is. A fair few of the smaller things don't make that much difference, but a more used to demonstrate what I was talking about. On Sun, 2005-07-17 at 13:21 -0400, Colin Walters wrote: > On Sun, 2005-07-17 at 02:06 +1000, James Livingston wrote: > > b) remove some pointless re-allocation of memory; this is mostly where > > we duplicate a string, call a function that will copy it, and then > > release our duplicate. > > This is generally a good idea as long as the allocation is actually in a > kind of memory hot-spot; e.g. if it's happening for each song or > something. But I don't think e.g. making these kinds of changes for > option processing or something is going to be a win if it makes the code > messier. I haven't looked at what you're changing, just making the > general point. Most of those probably don't make much difference, so I don't know whether they'd really be worth committing. There are quite a few places where we basically do the following: copy = strdup (somestring); func (copy); free (copy); Under most circumstances this could just be reduced to func(somestring), with no side-effects, and it doesn't really make it messier. But I don't think we need to bother with most of them, unless we're changing nearby code anyway. > > c) use memchunks in more places: for sequence nodes, rhythmdb actions > > and rhythmdb events. This helps lower heap fragmentation (and reduces RB > > total heap size, because of it). > > There was some interesting discussion re memchunks lately among the GLib > developers; some felt that it was actually worse than a good malloc > implementation. It's something we need to investigate: > > http://bugzilla.gnome.org/show_bug.cgi?id=118439#c19 > > Your point about fragmentation is good though, not sure it was really > addressed in that discussion. Hmmm, I didn't realise memchunks were that slow. The main reason was it helped to reduce the memory fragmentation, which was pretty bad. See below for more on the fragmentation issue. The one thing I think they do really help with is sequence nodes, because they are small and we use heaps of them. > > d) actually call g_blow_chunks() so that empty memchunks are released. > > You've gotta love that function name :) > > When do you call this? I put it in rhythmdb_idle_poll_events(), not because it was the best place, but it was in the rhythmdb code (the memchunks are in there) and it gets called regularly. Calling it is important if we use memchunks for things that are allocated/deallocated regularly in reasonable amounts; but it's not that important when we only used them in property-model. > > e) not commit the db until it is completely loaded, rather than after > > each entry. This means no entries will be stat'd until after it is > > loaded, which could be bad if the xml files is on a slow media. > > I don't understand this one; why shouldn't we be doing the stats in > parallel with loading? What happens when loading is for each db entry we allocate (1) the RhythmDbEntry and assocated data (strings, etc) and then (2) the ACTION_STAT structure and data (strings, gnome_vfs_file_info). This causes the memory allocations to go 12121212... All of the (2) memory is deallocated once the stat is done, which leaves memory heavily fragmented. By not calling commit after each entry the memory allocations go 1111...2222...., which doesn't leave the memory as fragmented. The fragmentation in the first way is slightly mitigated by using memchucks, but not by that much. The first method does mean that the stats are delayed until the xml has been completely processed, but the xml processing doesn't take that long (I don't have exact numbers). If it is only a small fraction of a second, it might be worth it to reduce the fragmentation - I'll have to look into it more. > Do you know what kind of performance impact fragmentation has? I > understand the concept but I'm not very familiar with what kind of > impact it typically has. Not specifically, I guess it would depend on exactly how glib implements malloc. In general I'd say that having lots (thousands) of small bits of free space would slow down locating one of the correct size; there are ways of dealing with this, with buckets for different free sizes and such, but I don't know what glib does. Changing the db commit from per-entry, to after-all-entries, did have a small but noticeable improvement in startup time. I don't know whether this is due to the reduction in fragmentation, or that calling commit once for X changed entries is faster than calling commit X times for one entry. Possibly there is some kind of tradeoff where we call commit after every N entries. This would only delay the start of the stats for a very short amount of time, but would help reduce the fragmentation. > > f) use RbRefStrings in a couple of places, rather than getting the > > contained string and then duplicating it. This changes the return type > > of the get_artist and get_album methods of RbSource, but these are never > > actually called anywhere, and library-source is the only thing that > > implements them. > > This is the kind of change I don't think we should be making...we should > identify the hotspots and fix those, not change all of the code. One thing I noticed is that rb-property-model gets strings out of RhythmDbEntries (which are RbRefStrings) and then copies them into it's own reference-counted string structure. AFAICT this is duplicating the names of every artist/album/genre needlessly - I don't know if there is a reason why it doesn't just use RbRefStrings. > > Action processing is in a thread and event processing is in a > > low-priority idle function; for me (on a single processor system) this > > means that all the action will be processed, before any of the events > > are. A stat event is fairly big (around 1k each, all up) so right > > before the events get processed 6.5Mb of the heap is in use; after they > > get processed (and freed) heap use is only 3Mb. With a SMT/SMP system > > they might be able to be run in parallel so this backlog doesn't occur, > > but I don't have one to check on. > > What if we limited the size of the action queue? Should be easy to do. > Actually I'm working on this code now, I might do that while I'm at it. > The potential cost here is that it could block the UI if the action > thread isn't scheduled, which to the user looks pretty bad. This might not be as important as it sounded, most of that 3.5Mb is taken up by the queries that run next, and the rest by GStreamer stuff when you start playing music. > > One idea that I had earlier today was that is RB was not active > > (minimised to the tray) we don't really need to keep all the queries > > active, we can destroy most of them. The only one we need is the one we > > are playing from, and we could even get rid of that if playback is > > stopped. > > Makes sense with the caveat that to the user it could be kind of > annoying if they switch back to their music and see their song list > visibly refilling. I'll have to see how expensive queries are CPU-wise; if they don't take that long to run it might be worth it - but it would be a tradeoff. Cheers, James "Doc" Livingston -- September 25th: Discovered lots of things about Dynamic HTML. Notably that almost every site attempting to use it is crap. -- Alan Cox's Diary
Attachment:
signature.asc
Description: This is a digitally signed message part