Review Request 109341: Optimizations to gwenview
Raul Fernandes
rgfernandes at gmail.com
Fri Nov 15 21:09:00 UTC 2013
> 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.
>
> Raul Fernandes wrote:
> 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 ??).
One thing I forgot to mention.
Using reserve and avoiding reallocations, the memory will be less fragmentated and there will be fewer memory accesses.
To x86_64, it is insignificant (I think), but for some architetures like ARM (small and slow memory) it matters.
- 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/1ed36a9b/attachment-0001.html>
More information about the Gwenview-devel
mailing list