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

[David Faure]

> On July 16, 2016, 9:44 p.m., Mark Gaiser wrote:
> > src/kiconloader.cpp, lines 342-345
> > <https://git.reviewboard.kde.org/r/128465/diff/1/?file=471855#file471855line342>
> >
> >     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 :)

src/corelib/tools/tools.pri is clear: on unix, qelapsedtimer_unix.cpp is used. In any case the intent is for this code to be cross-platform ;)

I'm using this same code in ksycoca and I heavily tested it there, I would doubt that this code never restarts the timer. But let's dig in to be sure.

The documentation for "qint64 restart()" is: "restarts the timer and returns the time elapsed since the previous start. This function is equivalent to obtaining the elapsed time with elapsed() and then starting the timer again with start(), but it does so in one single operation, avoiding the need to obtain the clock value twice."

So clearly start() can be used on an already running QElapsedTimer, it simply restarts it without returning the current elapsed time.

Seems fine to me.


- David


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


On July 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 July 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/e485de97/attachment.html>