Review Request 109341: Optimizations to gwenview

[Raul Fernandes]

> On March 18, 2013, 2:16 p.m., Aurélien Gâteau wrote:
> > Hi Raul, Thank you for your patch and sorry for the late answer.
> > 
> > I was about to report your const ref changes made the code reference temporary objects, but some further reading taught me this is actually OK in C++. One never stop to learn :)
> > 
> > I am not convinced with the calls to QList::reserve() though. Did you notice significant improvements with these changes? I wrote a quick bench for QList ( https://gist.github.com/agateau/5187229 ) and the difference between inserting 10000000 big structs with or without calling reserve was insignificant. I am reluctant to apply those changes if they don't bring anything.
> > 
> > Regarding optimizing ThumbnailViewPrivate::generateThumbnailsForItems(), I guess you actually mean optimizing ThumbnailLoadJob::appendItem(). That would make sense indeed, especially for big folders.
> > 
> > Some coding style nitpicks regarding your changes:
> > - Keep the "&" and the "*" with the type, not with the variable
> >   ie: "Foo* foo", not "Foo *foo"
> > - Keep opening brackets on the same line has their "creator"
> >   ie: "if (foo) {",
> >   not "if (foo)
> >   {"
> > - Avoid spaces inside parenthesis
> >   ie: "func(a, b)", not "func( a, b )"
> 
> Benni Hill wrote:
>     Regarding ThumbnailLoadJob::appendItem():
>     Please have a look at review #108843. I changed it to take a list if items and used a QMap to lookup if the items already exist.

Sorry for delayed answer.
I've look the code in your benchmark and run it in valgrind.
With the results, I noted that you looked in wrong place.
Most of the time the program is running in another place.
Not in QList<int>::append().
So the execution time has no difference.
I've run with 10.000.000 iterations, 9 iterations and 6 iterations.
The QList code allocates at least 8 bytes.
So with 9 iterations it has to realloc 1 time. With 6, no reallocs.

The results are:

10.000.000 iterations

no reserve
Total 460.836.014 (instructions)
QList<int>::append(int const&) 380.024.658
QListData::realloc(int) 23.477 (28 calls)

reserve
Total 460.814.527
QList<int>::append(int const&) 380.000.827
QListData::realloc(int) 2.143 (5 calls)

9 iterations

no reserve
Total 10.809.289
QList<int>::append(int const&) 3.410
QListData::realloc(int) 3.131 (8 calls)

reserve
Total 10.809.206
QList<int>::append(int const&) 1.169
QListData::realloc(int) 2.143 (5 calls)

6 iterations

no reserve
Total 10.809.154
QList<int>::append(int const&) 3.269
QListData::realloc(int) 3.131 (8 calls)

reserve
Total 10.808.754
QList<int>::append(int const&) 1.055
QListData::realloc(int) 2.143 (5 calls)

Strangely, with 6 iterations it has 8 calls to realloc.
Note that there are 5 calls in another place.
If I put reserve in these places, the reallocs will be zero.
With reserve, there are no reallocs (only that 5 calls).
The change seems to be too small but it is simple and free (no changes in algorithm) and could lower the (visible) latency of several operations.
Even with a small number of iterations, reserve gives some gains.
I will made the changes to patch to conform with coding style and updates to newer version.

I've found the problem with QUrl::operator==().
It uses thread locks for this operation and several others.
This is why it is so slow.
Maybe you can use another class to save the path of image (QFile ??).


- Raul


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109341/#review29443
-----------------------------------------------------------


On March 7, 2013, 9:46 p.m., Raul Fernandes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109341/
> -----------------------------------------------------------
> 
> (Updated March 7, 2013, 9:46 p.m.)
> 
> 
> Review request for Gwenview.
> 
> 
> Repository: gwenview
> 
> 
> Description
> -------
> 
> - Use const whenever possible (help the compiler optimize even more)
> - Reserve space in QList if we already know how many items will be added (avoid reallocations)
> - In JpegPlugin::keys(), make the variable static because this function is called many times and the contents are constant
> 
> These are simple optimizations that are safe to do.
> Using the profiler, I saw that one of the most expensive operations is QUrl::operator==() in ThumbnailViewPrivate::generateThumbnailsForItems().
> I think you can add the file to generate the thumbnail without checking if it is already added (using the cache to avoid to regenerate the thumbnail) or use a binary tree to check it.
> 
> 
> Diffs
> -----
> 
>   app/contextmanager.cpp 8e6d15e 
>   app/fileopscontextmanageritem.cpp 1408635 
>   app/fullscreencontent.cpp 9dfcabe 
>   app/main.cpp 446d428 
>   app/mainwindow.cpp 4ebef04 
>   importer/thumbnailpage.cpp 10c57d1 
>   lib/document/documentfactory.cpp 2906553 
>   lib/imageformats/jpegplugin.cpp ceb9b1c 
>   lib/jpegcontent.cpp e834399 
>   lib/semanticinfo/nepomuksemanticinfobackend.cpp 606214c 
>   lib/semanticinfo/tagwidget.cpp 4f41626 
>   lib/thumbnailview/thumbnailview.cpp 55a9006 
> 
> Diff: http://git.reviewboard.kde.org/r/109341/diff/
> 
> 
> Testing
> -------
> 
> Tested here and see no problems.
> 
> 
> Thanks,
> 
> Raul Fernandes
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/gwenview-devel/attachments/20131115/b21edd83/attachment.html>