Review Request 112253: KStandardItemListWidgetInformant/KFileItemListWidgetInformant: do not call the model's data(int) method if only the "text" is needed

[Emmanuel Pescosta]

> On Aug. 27, 2013, 7:22 p.m., Emmanuel Pescosta wrote:
> > dolphin/src/kitemviews/kfileitemlistwidget.cpp, line 42
> > <http://git.reviewboard.kde.org/r/112253/diff/1/?file=184445#file184445line42>
> >
> >     Why two casts?
> >     
> >     Wouldn't it be better if you do the type cast first and check it afterwards?
> >     
> >     KFileItemModel* fileItemModel = qobject_cast<KFileItemModel*>(view->model());
> >     Q_ASSERT(fileItemModel);
> 
> Frank Reininghaus wrote:
>     The point is that both casts have advantages and disadvantages:
>     
>     qobject_cast returns 0 if the argument cannot be cast successfully to the given type -> the assert will fail if the model is for some reason not a KFileItemModel. However, it is quite slow because of the safety that this runtime check provides.
>     
>     static_cast is very fast, but it will return a bogus KFileItemModel* pointer if the argument cannot be cast successfully. The behavior of the program is then undefined - most likely, it will crash very soon, but in principle, any kind of mysterious misbehavior could happen.
>     
>     The idea of the double cast (which I copied from Peter's code in KFileItemListView) is that it will only do the "fast" cast in release mode (asserts disabled), but it will do all possible checks in debug mode (asserts enabled).
>     
>     I did some experiments by enabling the benchmark output in the layouter in a release build, and then opening a folder with 100000 items 5 times in Icons View. It is indeed a bit slower when using qobject_cast (on average 1885 ms) instead of static_cast (1864 ms). However, both versions are still faster than current master (1927 ms), probably because calling KFileItem::text() is just faster than retrieving the "text" from a QHash (which involves calling qHash()) and then converting the QVariant to a string.

Great idea. Thanks for the explanation!

It's always great to learn new things :)


> On Aug. 27, 2013, 7:22 p.m., Emmanuel Pescosta wrote:
> > dolphin/src/kitemviews/kstandarditemlistwidget.cpp, line 103
> > <http://git.reviewboard.kde.org/r/112253/diff/1/?file=184447#file184447line103>
> >
> >     Const Ref
> 
> Frank Reininghaus wrote:
>     One could of course make this local variable a const reference, but note that "view->model()->data(index)" calls a virtual method which returns a QHash<QByteArray, QVariant>. Therefore, I think that the compiler has no chance at all to make use of the reference. It *must* create the QHash and then make the variable a reference to it. Most likely, the generated code will be exactly the same.
>     
>     I prefer to not make it a reference because this would give the wrong impression that there is a "cheap" reference to actual QHash which lives in the model, which is just wrong. (Note however, that copying the QHash is not really "expensive" either because it's implicitly shared with the one that is stored inside the model).

> but note that "view->model()->data(index)" calls a virtual method which returns a QHash<QByteArray, QVariant>.
Oh ok, so close this issue.


> On Aug. 27, 2013, 7:22 p.m., Emmanuel Pescosta wrote:
> > dolphin/src/kitemviews/kstandarditemlistwidget.cpp, line 165
> > <http://git.reviewboard.kde.org/r/112253/diff/1/?file=184447#file184447line165>
> >
> >     .value("text") instead of ["text"]
> 
> Frank Reininghaus wrote:
>     Hm, basically I copied the operator[] approach from the existing code in KStandardItemListWidgetInformant::itemSizeHint(). I think it doesn't hurt to use value() instead - it removes the ambiguity that there exist two overloads of operator[], which either return a value or a reference. But the generated code will probably be the same.
>     
>     Or is there another advantage of value() vs. operator[]?

More a matter of taste, but in this case I think .value() instead of [] is better readable.

But if you prefer the square brackets way, then remove this issue ;)


- Emmanuel


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


On Aug. 24, 2013, 6:54 p.m., Frank Reininghaus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112253/
> -----------------------------------------------------------
> 
> (Updated Aug. 24, 2013, 6:54 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Description
> -------
> 
> As I said on the mailing list some time ago, I think we can reduce the memory usage considerably by lazy-loading most of the data for each item. However, this only makes sense if we reduce the number of calls to the model's data() method, which constructs the full QHash containing all data, as much as possible.
> 
> Currently, KStandardItemListWidgetInformant calls the data() method for every single item, but the full data is actually only needed for the size calculation in Compact View. In Details View, no data are needed at all to determine the size required for the item, and in Icons View, only the name is needed.
> 
> This patch makes it possible for subclasses of KStandardItemListWidgetInformant to provide an alternative way to obtain the "text", to prevent calling the data() method of the model.
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kfileitemlistwidget.h 24c6778 
>   dolphin/src/kitemviews/kfileitemlistwidget.cpp 3a77241 
>   dolphin/src/kitemviews/kstandarditemlistwidget.h 4bf6116 
>   dolphin/src/kitemviews/kstandarditemlistwidget.cpp 2a89004 
> 
> Diff: http://git.reviewboard.kde.org/r/112253/diff/
> 
> 
> Testing
> -------
> 
> Works. No regressions found so far.
> 
> 
> Thanks,
> 
> Frank Reininghaus
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20130828/2c99e29e/attachment.htm>