Review Request: Some speed improvements in KFileItemModelRolesUpdater

[Frank Reininghaus]

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


Thanks Emmanuel! This is really nice work. I wasn't aware that the rehashing of the sets uses so many CPU cycles, and I think neither was Peter, who wrote this class.

I noticed that KFileItemModelRolesUpdater::sortAndResolvePendingRoles() does not seem to be a real bottleneck according to your data, so I first wondered if we really should make this function more complex than it is at the moment. However, I think that the statistics might look different if the user scrolls the view a lot while the previews are generated, so this makes sense.

I've added a few comments below, but the patch already looks very good, and I'm sure that the users will appreciate the performance improvements :-)




dolphin/src/kitemviews/kfileitemmodelrolesupdater.h
<http://git.reviewboard.kde.org/r/106577/#comment15371>

    Probably, it's mostly a matter of taste if one adds a timer as a member variable for this (as you suggest) or uses the static function QTimer::singleShot() (as it is currently). Is there a good reason to add the member? If not, I would actually prefer to leave it as it is because adding and handling the member requires more code.
    
    Right now, only two lines of code are needed for the two calls to QTimer::singleShot().



dolphin/src/kitemviews/kfileitemmodelrolesupdater.cpp
<http://git.reviewboard.kde.org/r/106577/#comment15372>

    These deletes are not needed. All these objects are children of the class, so they are deleted on destruction anyway.
    
    See http://qt-project.org/doc/qt-4.8/qobject.html#dtor.QObject



dolphin/src/kitemviews/kfileitemmodelrolesupdater.cpp
<http://git.reviewboard.kde.org/r/106577/#comment15373>

    Oops, good catch! This looks like you've found a real bug here. Can you commit this one-line change separately (and also to KDE/4.9)?



dolphin/src/kitemviews/kfileitemmodelrolesupdater.cpp
<http://git.reviewboard.kde.org/r/106577/#comment15374>

    I would add a comment here:
    
    // Step 1: Check if any items in m_pendingVisibleItems are not visible any more and move them to m_pendingInvisibleItems.



dolphin/src/kitemviews/kfileitemmodelrolesupdater.cpp
<http://git.reviewboard.kde.org/r/106577/#comment15375>

    Analogous:
    
    // Step 2: Check if any ...


- Frank Reininghaus


On Sept. 25, 2012, 9:07 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106577/
> -----------------------------------------------------------
> 
> (Updated Sept. 25, 2012, 9:07 p.m.)
> 
> 
> Review request for Dolphin and Frank Reininghaus.
> 
> 
> Description
> -------
> 
> Some speed improvements in KFileItemModelRolesUpdater (see KCachegrind screenshots - example sortAndResolvePendingRoles):
> - Use QSet.erase() instead of QSet.remove() => no expensive Rehashing 
> - Get rid of += in sortAndResolvePendingRoles() 
> - Some other small changes
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kfileitemmodelrolesupdater.h cabb003 
>   dolphin/src/kitemviews/kfileitemmodelrolesupdater.cpp 6dba224 
> 
> Diff: http://git.reviewboard.kde.org/r/106577/diff/
> 
> 
> Testing
> -------
> 
> 
> Screenshots
> -----------
> 
> Dolphin Git Master without patch
>   http://git.reviewboard.kde.org/r/106577/s/734/
> Dolphin Git Master with patch
>   http://git.reviewboard.kde.org/r/106577/s/735/
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

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