KIconLoader: massive speed improvement for loading unavailable icons
Review Request #128465 - Created July 16, 2016 and submitted
|cfeck, drosca, mpyne, ogoffart|
Summary: The code said "unknown icons should be searched for anew each time [so that installing new icons works]". However this leads to massive performance issues when opening the file dialog in a dir with many files for which there is no mimetype icon. In my case, 293 callgrind.out.* files in one dir (it's ironic that they would create a huge performance issue...). To make both sides happy (installing new icons should still work, but still unknown icons shouldn't lead to a full disk search every time icon.isNull() or icon.pixmap() is called), let's re-resolve unknown icons again only after 5 seconds. Benchmark results for loading an unavailable icon : before: 43 msecs per iteration (1897 disk locations checked) after: 0.013 msecs per iteration (pixmap found in the on-disk cache) And the file dialog no longer crawls to a halt in the dir with 293 callgrind files. Test Plan: Reviewers: Subscribers:
(described in commit log)
|.constFind() .constEnd()||Mark Gaiser|
I like the idea, and the code changes LGTM. But I think the autotest can be improved by ensuring that the short-term caching of the 'unknown' status of a non-existent icon works, instead of just ensuring that the cache is ignored once enough time has elapsed. i.e. something like // find non-existent icon // verify its null // install test icon (don't reset time-to-elapse yet) // find previously-non-existent icon // verify it's reported as null (this means we're caching its status) // reset time-to-elapse to zero // find previously-non-existent icon again // verify it's valid and exists I suppose this would make it more of a case of how it's implemented than what it's meant to do, but since caching the unknown-edness of a non-existent icon is what actually gives us the speedup we want, I think it makes sense to make sure we don't accidentally break that later.
Just a small nitpick.
I like your solution, nicely done!
New line for the opening brace.
I don't know if this is as you intent it.
The call mLastUnknownIconCheck.start(); starts the timer, but does it "restart" the timer if it was already started after the 5 seconds check?
The QElapsedTimer::start and QElapsedTimer::restart functions are implemented differently (see http://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/tools/qelapsedtimer_unix.cpp for reference). I havent't tested if start and restart behave the same. If the do then your code does what you intent. But then i wonder why QElapsedTimer has two different implementations for the same logic.
If start doesn't restart when called again, then your code - after the 5 seconds define - always ends up outside the if. In that case replacing mLastUnknownIconCheck.start(); with mLastUnknownIconCheck.restart(); would make it work as intended.
Ahh, vague.. If you look at the qelapsedtimer_generic.cpp then you see that start just calls restart (is that being used for Linux or is qelapsedtimer_unix.cpp being used? This confuses me..). Anyhow, if the generic implementation is used then you're fine with the code as is :)
Implemented the unittest suggested by Michael Pyne, and added another lookup after installation, because I spotted a bug in the code, the iconname wasn't removed from mUnavailableIcons. Makes me wonder if this should be a QHash<QString, bool> instead to minimize hashing and lookups?
Revision 2 (+93 -12)
For this function, i would consider changing d->mUnavailableIcons.contains(name) to in iterator lookup like:
auto unavailableIconIter = d->mUnavailableIcons.constFind(name);
Then use d->mUnavailableIcons.erase(unavailableIconIter); where it needs to be erased.
Contains on d->mAvailableIcons is still fine since you only do one lookup on that member.
I see what you mean about the QHash<QString,bool> suggestion. It might be better to merge mAvailableIcons and mUnavailableIcons to a single hash table as you propose (maybe named mIconAvailability ?), which would help ensure we don't accidentally break the exclusivity logic between available and unavailable icons in the future. But the code looks correct here as well (and is holding up well in testing here) so I'll leave it up to you whether you want to pursue.