Optimizations to gwenview

Review Request #109341 - Created March 7, 2013 and updated

Information
Raul Fernandes
gwenview
KDE/4.10
Reviewers
gwenview
- 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.
Tested here and see no problems.

Issues

  • 1
  • 0
  • 0
  • 1
Description From Last Updated
Could be further simplified to: static QStringList list = QStringList() << QLatin1String("jpeg") << QLatin1String("jpg"); Aurélien Gâteau Aurélien Gâteau
Raul Fernandes
Review request changed
Aurélien Gâteau
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 )"
  1. 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.
  2. 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 ??).
  3. 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.
lib/imageformats/jpegplugin.cpp (Diff revision 1)
 
 
Could be further simplified to:
static QStringList list = QStringList() << QLatin1String("jpeg") << QLatin1String("jpg");
Loading...