> On jul 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 :) > > David Faure wrote: > 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.
Sounds like it's all ok then :) Thank you for digging into that. +1 - Mark ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128465/#review97486 ----------------------------------------------------------- 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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
