<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/109341/">http://git.reviewboard.kde.org/r/109341/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 18th, 2013, 2:16 p.m. UTC, <b>Aurélien Gâteau</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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 )"</pre>
</blockquote>
<p>On March 22nd, 2013, 10:44 p.m. UTC, <b>Benni Hill</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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 ??).</pre>
<br />
<p>- Raul</p>
<br />
<p>On March 7th, 2013, 9:46 p.m. UTC, Raul Fernandes wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Gwenview.</div>
<div>By Raul Fernandes.</div>
<p style="color: grey;"><i>Updated March 7, 2013, 9:46 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
gwenview
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">- 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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Tested here and see no problems.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>app/contextmanager.cpp <span style="color: grey">(8e6d15e)</span></li>
<li>app/fileopscontextmanageritem.cpp <span style="color: grey">(1408635)</span></li>
<li>app/fullscreencontent.cpp <span style="color: grey">(9dfcabe)</span></li>
<li>app/main.cpp <span style="color: grey">(446d428)</span></li>
<li>app/mainwindow.cpp <span style="color: grey">(4ebef04)</span></li>
<li>importer/thumbnailpage.cpp <span style="color: grey">(10c57d1)</span></li>
<li>lib/document/documentfactory.cpp <span style="color: grey">(2906553)</span></li>
<li>lib/imageformats/jpegplugin.cpp <span style="color: grey">(ceb9b1c)</span></li>
<li>lib/jpegcontent.cpp <span style="color: grey">(e834399)</span></li>
<li>lib/semanticinfo/nepomuksemanticinfobackend.cpp <span style="color: grey">(606214c)</span></li>
<li>lib/semanticinfo/tagwidget.cpp <span style="color: grey">(4f41626)</span></li>
<li>lib/thumbnailview/thumbnailview.cpp <span style="color: grey">(55a9006)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/109341/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>