Review Request 128465: KIconLoader: massive speed improvement for loading unavailable icons

[Mark Gaiser]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128465/#review97486
-----------------------------------------------------------



Just a small nitpick.
I like your solution, nicely done!


src/kiconloader.cpp (line 341)
<https://git.reviewboard.kde.org/r/128465/#comment65759>

    New line for the opening brace.



src/kiconloader.cpp (lines 342 - 345)
<https://git.reviewboard.kde.org/r/128465/#comment65760>

    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 :)


- Mark Gaiser


On jul 16, 2016, 9:52 a.m., David Faure wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128465/
> -----------------------------------------------------------
> 
> (Updated jul 16, 2016, 9:52 a.m.)
> 
> 
> Review request for KDE Frameworks, Christoph Feck, David Rosca, Michael Pyne, and Olivier Goffart.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> -------
> 
> 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:
> 
> 
> Diffs
> -----
> 
>   autotests/kiconengine_unittest.cpp 105e86c1e7bc6170c626aa80d34cdb6422acca9c 
>   src/kiconloader.cpp d885318c166f2a996b038218366317615886a14e 
> 
> Diff: https://git.reviewboard.kde.org/r/128465/diff/
> 
> 
> Testing
> -------
> 
> (described in commit log)
> 
> 
> Thanks,
> 
> David Faure
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160716/5efeabcf/attachment.html>