| Summary: | Cover manager is slow/freezes | ||
|---|---|---|---|
| Product: | [Applications] amarok | Reporter: | Michael Reiher <redm> |
| Component: | general | Assignee: | Amarok Developers <amarok-bugs-dist> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | endrebjorsvik, fulmare90, jajaxor |
| Priority: | NOR | ||
| Version: | 2.3-GIT | ||
| Target Milestone: | --- | ||
| Platform: | Ubuntu | ||
| OS: | Unspecified | ||
| Latest Commit: | Version Fixed In: | ||
| Sentry Crash Report: | |||
| Attachments: | Improve covermanager startup and fix freezes. | ||
Probably a duplicate of bug 197242 Michael, I think you would make many people happy, (including me :P) if you could come up with a patch for this bug! As you said, loading the cover manager takes lot of memory: those with large collections like me end up with a trashed system, also because you can't cancel the cover manager loading (that's bug #204882 ). Thanks commit 26c2c503eabc4c098e3ab538753cbf85e91db71b Author: Mark Kretschmann <kretschmann@kde.org> Date: Wed Oct 14 11:46:26 2009 +0200 Remove pointless QPixmap conversion, which slowed down CoverManager. commit 52efeaa7d714994680cd0cb7a4145ab21782d0fd Author: Mark Kretschmann <kretschmann@kde.org> Date: Wed Oct 14 12:44:15 2009 +0200 Remove some unnecessary image scaling when loading CoverManager. CCBUG: 210454 What this class really needs is on-demand loading when you scroll, I think. That's the only way to really solve it. Perhaps I'm completely missing something here, so correct when I'm wrong, but your changes don't make much sense to me... Removing CoverViewItem::m_coverPixmap (holding the full sized image) was surely a good idea, as it wasn't used anyway and was the real cause for the tremendous memory usage. However then you used the full sized image instead of the size 100 one as item icon...? :) Well, in the end it used only a little bit less memory (the size 100 images) and was probably even a bit slower than before (2 times loading the full size image per album). Also it didn't affect my original problem, the 20s freezes, at all. I dug a bit more into the code and made a patch (attached to this report) which uses only a fraction of memory and loading time and also (almost) removes the freezes. Now I load only the size 100 images and removed loading the full size images entirely. The loadCover() in CoverViewItem() is completly redundant with m_coverPixmap removed. To remove the freezes I made a separate implementation for SqlAlbum::hasImage which doesn't load a large image just to check it's existence. IMHO it's better to use SqlAlbum::image() only when we really want an image. One thing left which I found confusing is the 60s timer in SqlRegistry which clears all caches, one of the reasons for the freezes. With the optimization of SqlAlbum::hasImage things are a lot better than before, there is still a little lag ever 60s (the constantly triggered updateStatusBar checks all album covers for existence). If there are questions or suggestions, I'm happy to hear! Created attachment 37602 [details]
Improve covermanager startup and fix freezes.
Patch against current Git.
Many thanks Michael, I've committed your patch: commit 9e36c0320b07a83b5ec796a1b463fca15013ebcc Author: Mark Kretschmann <kretschmann@kde.org> Date: Fri Oct 16 07:44:47 2009 +0200 Huge performance improvements for the Cover Manager. Thanks to Michael Reiher <redm@gmx.de> for the patch! BUG: 210454 PS: About the 60s timer clearing the cache, this sounds very odd to me too. I think we could reduce the frequency a lot. Maybe 10 minutes or so? thanks for the patch. Now my 1600 album covers are loaded almost immediately. But I do notice lots of disk activity shortly after. (In reply to comment #9) > PS: About the 60s timer clearing the cache, this sounds very odd to me too. I > think we could reduce the frequency a lot. Maybe 10 minutes or so? Well, don't ask me, why it's there and why it's 60s. I don't really have a clue here. :) But it seems to be a more general thing with the SQL collections, i.e. not related to the cover manager. Normally apparently not really noticeable. A thought I had was to perhaps suspend it, while the manager is up. (In reply to comment #10) > thanks for the patch. Now my 1600 album covers are loaded almost immediately. > But I do notice lots of disk activity shortly after. This probably has to do with the above mentioned timer. It periodically clears all info about the covers, so they have to be rechecked, which gives a little lag. Not nice, but at least a lot better than before ;) commit 5fdae6069cfb8723169f31f851c4caa3936b172f Author: Mark Kretschmann <kretschmann@kde.org> Date: Fri Oct 16 11:30:19 2009 +0200 Reduce SqlRegistry cache clearing frequency from 60s to 300s. This should improve performance somewhat, especially for cover image handling. CCBUG: 210454 *** Bug 212964 has been marked as a duplicate of this bug. *** Hi (sorry again and again for my bad english...) I never see this bug report sorry but A BIG thanks for your help and works ;) arghhh, I want update to 2.2.1 more now !!!!!!!!! *** Bug 215159 has been marked as a duplicate of this bug. *** |
Version: (using KDE 4.3.2) Installed from: Ubuntu Packages The initial problem is that the cover manager constantly freezes for about 20s. It works in general, but it's not exactly a pleasure to use. So I ran the cover manager through callgrind to find out where the time is spent. It showed that most of the time is spent in SqlAlbum::image() loading and creating QPixmaps. Further investigation showed that this also affacts the inital cover loading. Here are my findings. I'll first describe what I observed and then go on with the individual issues. Note that I'm not really familiar with the code, so I might be wrong here or there... That's also why I'd leave a proper fix for those in know. ;) Some data: ---------------- Using Amarok 2.2.0 (to be precise git from 2009-10-12) Athlon64 2.2GHz CoverManager says 334 covers. What happens: --------------------- Initial problem: - Cover manager has loaded covers and is running. - I browse covers by scrolling the view and moving the pointer across. - This repeatedly triggers CoverManager::updateStatusBar() (probably through CoverManager::slotArtistSelectedContinueAgain() or signal viewportEntered(), I did not check that) - To count missing covers it calls for every album: CoverViewItem::hasCover() -> SqlAlbum::hasImage() -> SqlAlbum::image(). In it: - No cached image found - Finds original cover image in /home/michael/Music/... via findImage() - creates QPixmap from potentially large image!!! (line: return QPixmap( result );) - This entire process takes about 20s here for all albums at full CPU, during which the cover manager is frozen - This expensive process is repeated for every updateStatusBar(), because: - In between them an invalidation of the loaded covers kicks in and clears the cached data (SqlAlbum::invalidateCache() for every album) Initial loading of covers (when cover manager is launched): - for every album the following two thinga are done: - CoverViewItem::CoverViewItem() loads covers of size 100 via SqlAlbum::image() (found in cover cache on disk via findCachedImage()) - CoverViewItem::loadCover() loads images of size 1 via SqlAlbum::image() - at some point during loading SqlAlbum::invalidateCache() kicks in (for every album) - after loading covers an additional CoverManager::updateStatusBar() is called (see above) I found the following (possible) problems: --------------------------------------------------------------- Problem 1) Checking whether an album has a cover image is not exactly efficient. It loads the *original* cover image from disk and creates a QPixmap from it. These images can be quite big. And only to check if something has been loaded... (isNull()) Problem 1a) Loading all these images uses an enormous amount of memory. E.g. most of my covers have a size of about 500x500 pixels by 4 byte by 334 covers this makes 334 MB of memory! This comes close to reality, when I look at the RES value of Xorg in top. It might also be, that not all memory is released afterwards, btw. (Also, I did not read it in detail, but maybe this is also the cause for Bug 197242?) Problem 2) As far as time is concerned this would be bearable, if the result would at least be cached properly. However while the cover manager is open, regularly SqlAlbum::invalidateCache() kicks in and clears all album objects. Which means on next CoverManager::updateStatusBar() all original covers are read again. Problem 3) Why does CoverManager::updateStatusBar() check all covers over and over again to count missing covers, even though nothing has changed i.e. I'm only browsing the view? Perhaps if efficient this is not so much a problem, but atm. it is. Problem 4) In CoverViewItem::loadCover() there is the line: m_coverPixmap = m_albumPtr->image(); //create the scaled cover What is this line good for? Does that do what it should?? It calls SqlAlbum::image() with the default size = 1. In there the original image is loaded and the "size < 1000" check returns it. Nothing is scaled. Even if it would pass the check and createScaledImage() is called (Btw: What if there is no large image?), this returns just an empty string for size == 1... Also the loaded image is invalidated sooner or later anyway... What ever it is supposed to do, it seems to do a lot of unneccessary processing in anycase (loading all the images for nothing). So far for now. If this was too confusing or there are other questions, just ask! :)