Review Request 127236: Leverage QDir to know what's in a KIconThemeDir

[Nick Shaforostoff]

> On March 1, 2016, 4:37 p.m., Aleix Pol Gonzalez wrote:
> > Eh... I just realized it's not 100% correct. We have a test (testUnknownIconNotCached) that fails, unsure how I didn't see it yesterday.
> > 
> > The problem with this one is that we are not reacting when icons are introduced at runtime. I tried adding a sneaky QFileSystemWatcher/KDirWatch on KIconTheme but it didn't signal.
> > 
> > Should I just discard the patch?
> 
> David Edmundson wrote:
>     Could you compare modified time of mBaseDirThemeDir and reload if needed?
>     it'd be one extra stat on the directory each time, but still a saving from before
> 
> Aleix Pol Gonzalez wrote:
>     That doesn't work, for some reason...
>     ```
>              // Install the existing icon by copying.
>     +        qDebug() << "xxxxxxxx1" << QFileInfo(actionIconsDir).lastModified();
>              QVERIFY(QFile::copy(QStringLiteral(":/test-22x22.png"), newIconPath));
>     +        qDebug() << "xxxxxxxx2" << QFileInfo(actionIconsDir).lastModified();
>     ```
>     
>     Both return the same value.
> 
> Nick Shaforostoff wrote:
>     there is a difference between changing a file from inside the program and changing it from outside.
>     
>     did you make the change externally when testing QFileSystemWatcher-based solution?
> 
> Nick Shaforostoff wrote:
>     also for the QFileInfo::lastModified() there may be just not enough time between the tests, so try adding a 2-minute sleep there
> 
> David Faure wrote:
>     QFileInfo::lastModified() doesn't include milliseconds yet. There is a pending patch for Qt to implement that (as part of a much bigger task which also adds QFile::setFileTime()), but yeah, that's the problem. Right now you need a 1s sleep whenever unittesting changes to lastModified().
>     
>         QTest::qWait(1000); // remove this once lastModified includes ms
>     
>     Or at least "up to 1s, until the end of the current second", I have code for that in the kdirwatch unittest for instance.
>     
>     Another alternative, hacking the modification time on files (using utime, like I do in ksycocatest.cpp)
>     
>     (all this to avoid tests that take 15 seconds in total, I like the policy that a test should run in under 5s -- otherwise people stop running them)
> 
> Nick Shaforostoff wrote:
>     so, any news on this one?

even simple solution of discarding the cache after certain point of time (e.g. 10 seconds) and going uncached code path would give us the benefit without introducing regression, right?


- Nick


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


On March 1, 2016, 3:25 a.m., Aleix Pol Gonzalez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127236/
> -----------------------------------------------------------
> 
> (Updated March 1, 2016, 3:25 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> -------
> 
> At the moment we're playing Battleship to see if an icon is present in a subdirectory. This means that we are checking on every directory if there's an icon that matches the size with a said name on every request.
> 
> This can be seen easily with strace:
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 6212
> ```
> After the patch: 
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 1993
> ```
> We reduce these accesses to let QDir keep the list of files inside the directory (that was already being generated at some point, it just was being discarded).
> 
> 
> Diffs
> -----
> 
>   src/kicontheme.cpp 0996054 
> 
> Diff: https://git.reviewboard.kde.org/r/127236/diff/
> 
> 
> Testing
> -------
> 
> Builds, tests still pass, applications start noticeably faster.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160314/1906593d/attachment.html>