-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128465/#review97478
-----------------------------------------------------------
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.
- Michael Pyne
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
>
>
_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
