Review Request 115064: Use only one "directory contents counting" thread per process

[Frank Reininghaus]

> On Jan. 17, 2014, 2:10 p.m., Mark Gaiser wrote:
> > +1 (as in looks good and sane to me. Not tested it though)
> > 
> > However! (i always seems to have a "however" in those review requests where i do respond :p)
> > It really smells like you need to use a std::shared_ptr. What you do right now "looks" like manual reference counting and destroying if it's 0. Well, that's about the description of shared pointers as well :)
> 
> Frank Reininghaus wrote:
>     Thanks for your comment. The patch is doing something very similar to what std::shared_ptr and QSharedPointer are designed for indeed. The reason why using one of these is not so straightforward here is that you can't just delete the QThread once the last pointer to it goes out of scope - you really have to finish the thread cleanly and wait until it's done by calling quit() and then wait(). If you don't do that and just delete it anyway, the process might crash if the thread is still running.
>     
>     If one really wanted to use a shared pointer, then one would have to implement a class that wraps the QThread, and that calls its quit() and wait() methods in its destructor. This is definitely possible, but I thought that just doing the reference counting manually is easier.
> 
> Mark Gaiser wrote:
>     Thank you very much for the explanation!
>     Does that mean that QThread can't - because of it's design - ever be used in any of the smart pointers?

I think that using smart pointers to a QThread is indeed unsafe, unless you can guarantee that the thread is not running any more when the smart pointer deletes it.


- Frank


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115064/#review47571
-----------------------------------------------------------


On Jan. 17, 2014, 8:23 a.m., Frank Reininghaus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115064/
> -----------------------------------------------------------
> 
> (Updated Jan. 17, 2014, 8:23 a.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Repository: kde-baseapps
> 
> 
> Description
> -------
> 
> Since https://git.reviewboard.kde.org/r/111920/, we use a separate thread for counting the items inside directories (e.g., for the "Size" column in Details View) to prevent that the GUI is blocked while handling large directories. However, this means that Dolphin now uses quite a lot of threads if there are many views. To see that, run Dolphin in gdb, open many tabs with Ctrl+T, and check the output of "thread apply all backtrace.
> 
> I propose to make all views share the same thread. The QThread object is stored in a global variable, and each view increments/decrements a reference count when it starts/stops using the thread. If this thread reaches zero, the thread is stopped.
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/private/kdirectorycontentscounter.h 425c363 
>   dolphin/src/kitemviews/private/kdirectorycontentscounter.cpp fd8479f 
> 
> Diff: https://git.reviewboard.kde.org/r/115064/diff/
> 
> 
> Testing
> -------
> 
> Opening new views does not start new threads any more. The "Size" column in Details view still works normally for me.
> 
> 
> Thanks,
> 
> Frank Reininghaus
> 
>

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