Every time I change the active torrent to one which has 5070 files in 71 directories, it takes a while for KTorrent to become responsive again.

Please make the infowidget plugin to background the file tree creation or only update the active tab the same moment the torrent changes. Thanks.

5070, I can see that taking a while.

The problem is that doing this in a background thread is difficult with Qt (if not impossible)

3 hours coding + 2 hours bugfixing resulted in the following:

Code: Select all

Index: plugins/infowidget/fileviewupdatethread.h
===================================================================
--- plugins/infowidget/fileviewupdatethread.h   (revision 0)
+++ plugins/infowidget/fileviewupdatethread.h   (revision 0)
@@ -0,0 +1,57 @@
+/***************************************************************************
+ *   Copyright (C) 2007 by Jaak Ristioja                                   *
+ *   Ristioja@gmail.com                                                    *
+ *                                                                         *
+ *   This program is free software; you can redistribute it and/or modify  *
+ *   it under the terms of the GNU General Public License as published by  *
+ *   the Free Software Foundation; either version 2 of the License, or     *
+ *   (at your option) any later version.                                   *
+ *                                                                         *
+ *   This program is distributed in the hope that it will be useful,       *
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of        *
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the         *
+ *   GNU General Public License for more details.                          *
+ *                                                                         *
+ *   You should have received a copy of the GNU General Public License     *
+ *   along with this program; if not, write to the                         *
+ *   Free Software Foundation, Inc.,                                       *
+ *   51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.          *
+ ***************************************************************************/
+#ifndef KTFILEVIEWUPDATETHREAD_H
+#define KTFILEVIEWUPDATETHREAD_H
+
+#include <qthread.h>
+
+namespace kt
+{
+   class FileView;
+   class TorrentInterface;
+   
+   class FileViewUpdateThread : public QThread
+   {
+      public:
+         FileViewUpdateThread(FileView* fileview);
+         ~FileViewUpdateThread();
+         
+         void start(kt::TorrentInterface* new_tc, QThread::Priority priority);
+         void run();
+         
+         /**
+            Stops the running thread. It will do this in the background so when
+            returning from this function, the thread might not be completely
+            stopped yet. This is done as a safety measure to prevent failure in
+            arbitrary thread termination.
+         */
+         void stop();
+      
+      private:
+         bool fillFileTree();
+      
+      private:
+         FileView* fileview;
+         kt::TorrentInterface* new_tc;
+         bool stopThread;
+   };
+};
+
+#endif
Index: plugins/infowidget/fileview.h
===================================================================
--- plugins/infowidget/fileview.h   (revision 727641)
+++ plugins/infowidget/fileview.h   (working copy)
@@ -21,12 +21,16 @@
 #define KTFILEVIEW_H
 
 #include <klistview.h>
+#include <qmutex.h>
 #include <util/constants.h>
 
+class KPopupMenu;
+
 namespace kt
 {
    class TorrentInterface;
    class IWFileTreeDirItem;
+   class FileViewUpdateThread;
 
    /**
       @author Joris Guisson <joris.guisson@gmail.com>
@@ -35,11 +39,17 @@
    {
       Q_OBJECT
    public:
+      friend class FileViewUpdateThread;
+      
       FileView(QWidget *parent = 0, const char *name = 0);
       virtual ~FileView();
 
       void update();
       void changeTC(kt::TorrentInterface* tc);
+   
+   protected:
+      virtual void viewportPaintEvent(QPaintEvent* pe);
+   
    private slots:
       void contextItem(int id);
       void showContextMenu(KListView* ,QListViewItem* item,const QPoint & p);
@@ -47,7 +57,6 @@
       void onDoubleClicked(QListViewItem* item,const QPoint & ,int );
       
    private:
-      void fillFileTree();
       void readyPreview();
       void readyPercentage();
       void changePriority(QListViewItem* item, bt::Priority newpriority);
@@ -56,7 +65,9 @@
       kt::TorrentInterface* curr_tc;
       IWFileTreeDirItem* multi_root;
       KPopupMenu* context_menu;
+      FileViewUpdateThread* update_thread;
       QString preview_path;
+      QMutex eventlock;   // To avoid segfaults when events reach the listview while updating in background
       int preview_id;
       int first_id;
       int normal_id;
Index: plugins/infowidget/fileviewupdatethread.cpp
===================================================================
--- plugins/infowidget/fileviewupdatethread.cpp   (revision 0)
+++ plugins/infowidget/fileviewupdatethread.cpp   (revision 0)
@@ -0,0 +1,129 @@
+/***************************************************************************
+ *   Copyright (C) 2007 by Jaak Ristioja                                   *
+ *   Ristioja@gmail.com                                                    *
+ *                                                                         *
+ *   This program is free software; you can redistribute it and/or modify  *
+ *   it under the terms of the GNU General Public License as published by  *
+ *   the Free Software Foundation; either version 2 of the License, or     *
+ *   (at your option) any later version.                                   *
+ *                                                                         *
+ *   This program is distributed in the hope that it will be useful,       *
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of        *
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the         *
+ *   GNU General Public License for more details.                          *
+ *                                                                         *
+ *   You should have received a copy of the GNU General Public License     *
+ *   along with this program; if not, write to the                         *
+ *   Free Software Foundation, Inc.,                                       *
+ *   51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.          *
+ ***************************************************************************/
+#include <kiconloader.h>
+#include <kmimetype.h>
+#include <interfaces/functions.h>
+#include <interfaces/torrentinterface.h>
+#include <interfaces/torrentfileinterface.h>
+#include "iwfiletreeitem.h"
+#include "iwfiletreediritem.h"
+#include "fileview.h"
+#include "fileviewupdatethread.h"
+
+namespace kt
+{
+   FileViewUpdateThread::FileViewUpdateThread(FileView* fileview)
+      : fileview(fileview), stopThread(true)
+   {
+      // empty
+   }
+   
+   FileViewUpdateThread::~FileViewUpdateThread()
+   {
+      // empty
+   }
+   
+   void FileViewUpdateThread::start(kt::TorrentInterface* new_tc, QThread::Priority priority) {
+      stopThread = false;
+      this->new_tc = new_tc;
+      QThread::start(priority);
+   }
+   
+   void FileViewUpdateThread::run()
+   {
+      fileview->setEnabled(false);
+      
+      if (!fillFileTree()) return;
+      
+      if (new_tc != 0) {
+         QObject::connect(
+               new_tc,
+                SIGNAL(missingFilesMarkedDND( kt::TorrentInterface* )),
+               fileview,
+                SLOT(refreshFileTree( kt::TorrentInterface* ))
+            );
+         fileview->setEnabled(true);
+      }
+   }
+   
+   void FileViewUpdateThread::stop()
+   {
+      stopThread = true;
+   }
+   
+   bool FileViewUpdateThread::fillFileTree()
+   {
+      fileview->eventlock.lock();
+      fileview->curr_tc = new_tc;
+      fileview->multi_root = 0;
+      fileview->clear();
+      fileview->eventlock.unlock();
+      
+      if (!fileview->curr_tc)
+         return true;
+      
+      if (fileview->curr_tc->getStats().multi_file_torrent)
+      {
+         fileview->eventlock.lock();
+         IWFileTreeDirItem* root = new IWFileTreeDirItem(
+               fileview,
+               fileview->curr_tc->getStats().torrent_name
+            );
+         fileview->eventlock.unlock();
+         
+         for (Uint32 i = 0; i < fileview->curr_tc->getNumFiles(); i++)
+         {
+            TorrentFileInterface & file = fileview->curr_tc->getTorrentFile(i);
+            
+            fileview->eventlock.lock();
+            root->insert(file.getPath(), file);
+            if (stopThread) {
+               fileview->clear();
+               fileview->eventlock.unlock();
+               return false;
+            }
+            fileview->eventlock.unlock();
+         }
+         fileview->eventlock.lock();
+         root->setOpen(true);
+         fileview->setRootIsDecorated(true);
+         fileview->multi_root = root;
+         fileview->multi_root->updatePriorityInformation(fileview->curr_tc);
+         fileview->multi_root->updatePercentageInformation();
+         fileview->multi_root->updatePreviewInformation(fileview->curr_tc);
+         fileview->eventlock.unlock();
+      }
+      else
+      {
+         const TorrentStats & s = fileview->curr_tc->getStats();
+         fileview->eventlock.lock();
+         fileview->setRootIsDecorated(false);
+         KListViewItem* item = new KListViewItem(
+               fileview,
+               s.torrent_name,
+               BytesToString(s.total_bytes)
+            );
+         
+         item->setPixmap(0, KMimeType::findByPath(s.torrent_name)->pixmap(KIcon::Small));
+         fileview->eventlock.unlock();
+      }
+      return true;
+   }
+} // namespace kt
Index: plugins/infowidget/fileview.cpp
===================================================================
--- plugins/infowidget/fileview.cpp   (revision 727641)
+++ plugins/infowidget/fileview.cpp   (working copy)
@@ -17,13 +17,12 @@
  *   Free Software Foundation, Inc.,                                       *
  *   51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.          *
  ***************************************************************************/
-#include <klocale.h>
 #include <kiconloader.h>
+#include <klocale.h>
 #include <kglobal.h>
 #include <kpopupmenu.h>
 #include <krun.h>
 #include <kmessagebox.h>
-#include <kmimetype.h>
 #include <util/bitset.h>
 #include <util/functions.h>
 #include <interfaces/functions.h>
@@ -33,6 +32,7 @@
 #include "functions.h"
 #include "iwfiletreeitem.h"
 #include "iwfiletreediritem.h"
+#include "fileviewupdatethread.h"
 #include "fileview.h"
       
 using namespace bt;
@@ -43,6 +43,8 @@
    FileView::FileView(QWidget *parent, const char *name)
          : KListView(parent, name),curr_tc(0),multi_root(0)
    {
+      update_thread = new FileViewUpdateThread(this);
+      
       setFrameShape(QFrame::NoFrame);
       addColumn( i18n( "File" ) );
        addColumn( i18n( "Size" ) );
@@ -83,57 +85,25 @@
 
 
    FileView::~FileView()
-   {}
-   
-   void FileView::fillFileTree()
    {
-      multi_root = 0;
-      clear();
-   
-      if (!curr_tc)
-         return;
-   
-      if (curr_tc->getStats().multi_file_torrent)
-      {
-         IWFileTreeDirItem* root = new IWFileTreeDirItem(
-               this,curr_tc->getStats().torrent_name);
-         
-         for (Uint32 i = 0;i < curr_tc->getNumFiles();i++)
-         {
-            TorrentFileInterface & file = curr_tc->getTorrentFile(i);
-            root->insert(file.getPath(),file);
-         }
-         root->setOpen(true);
-         setRootIsDecorated(true);
-         multi_root = root;
-         multi_root->updatePriorityInformation(curr_tc);
-         multi_root->updatePercentageInformation();
-         multi_root->updatePreviewInformation(curr_tc);
+      if (update_thread) {
+         update_thread->stop();
+         update_thread->wait();
       }
-      else
-      {
-         const TorrentStats & s = curr_tc->getStats();
-         this->setRootIsDecorated(false);
-         KListViewItem* item = new KListViewItem(
-               this,
-               s.torrent_name,
-               BytesToString(s.total_bytes));
-   
-         item->setPixmap(0,KMimeType::findByPath(s.torrent_name)->pixmap(KIcon::Small));
-      }
+      delete update_thread;
    }
 
    void FileView::changeTC(kt::TorrentInterface* tc)
    {
       if (tc == curr_tc)
          return;
-   
-      curr_tc = tc;
-      fillFileTree();
-      setEnabled(tc != 0);
-      if (tc)
-         connect(tc,SIGNAL(missingFilesMarkedDND( kt::TorrentInterface* )),
-               this,SLOT(refreshFileTree( kt::TorrentInterface* )));
+      
+      if (update_thread) {
+         update_thread->stop();
+         update_thread->wait();
+      }
+      
+      update_thread->start(tc, QThread::LowPriority);
    }
    
    void FileView::update()
@@ -148,6 +118,12 @@
       }
    }
    
+   void FileView::viewportPaintEvent(QPaintEvent* pe) {
+      eventlock.lock();
+      KListView::viewportPaintEvent(pe);
+      eventlock.unlock();
+   }
+   
    void FileView::readyPercentage()
    {
       if (curr_tc && !curr_tc->getStats().multi_file_torrent)
Index: plugins/infowidget/Makefile.am
===================================================================
--- plugins/infowidget/Makefile.am   (revision 727641)
+++ plugins/infowidget/Makefile.am   (working copy)
@@ -2,12 +2,12 @@
 METASOURCES = AUTO
 kde_module_LTLIBRARIES = ktinfowidgetplugin.la
 noinst_HEADERS = infowidgetplugin.h infowidgetprefpage.h trackerview.h GeoIP.h \
-   statustab.h fileview.h
+   statustab.h fileview.h fileviewupdatethread.h
 ktinfowidgetplugin_la_SOURCES = infowidgetplugin.cpp availabilitychunkbar.cpp \
    chunkbar.cpp chunkdownloadview.cpp downloadedchunkbar.cpp flagdb.cpp peerview.cpp \
    ktorrentmonitor.cpp iwfiletreediritem.cpp iwfiletreeitem.cpp infowidgetprefpage.cpp \
    infowidgetpluginsettings.kcfgc iwpref.ui trackerviewbase.ui trackerview.cpp floatspinbox.cpp \
-   localefloatvalidator.cpp chunkdownloadviewbase.ui statustabbase.ui statustab.cpp fileview.cpp
+   localefloatvalidator.cpp chunkdownloadviewbase.ui statustabbase.ui statustab.cpp fileview.cpp fileviewupdatethread.cpp
 
 # Libs needed by the plugin
 ktinfowidgetplugin_la_LIBADD = ../../libktorrent/libktorrent.la \


The lock is used because otherwise Qt had some crashes because the listview items were changing while it tried to repaint the widget.

Also, it were trivial to modify this to only use a thread when a torrent consists of more than N files.

Edit: changed name of QMutex Fileview.lock to Fileview.eventlock.

Hopefully this doesn't crash, QObject subclasses are not thread safe. So this looks like a very dangerous patch.

Maybe Qt's event handling for FileView needs to be protected to by the mutex. Suppose you are adding child items to one item, and at the same time the user expands it. Qt's container classes are not thread safe.

Can you send that in a file to joris AT guisson DOT gmail DOT com ? Patches copy pasted from the forum have a tendency to not apply properly.

George wrote:Hopefully this doesn't crash, QObject subclasses are not thread safe. So this looks like a very dangerous patch.

Perhaps. I've done the following: keep the "files" tab open and scroll through the torrents by holding down an arrow key, and hopefully eliminated at least one bug I encountered while doing so.

George wrote:Maybe Qt's event handling for FileView needs to be protected to by the mutex. Suppose you are adding child items to one item, and at the same time the user expands it. Qt's container classes are not thread safe.

The KListView widget is disabled while adding items to the list, so the user can't expand parent items. Also there is a mutex preventing it from redrawing the widget while adding items. And if I get it right, the changeTC function nor the destructor of the Fileview class can be called in parallel, so that one doesn't need a mutex.

George wrote:Can you send that in a file to joris AT guisson DOT gmail DOT com ? Patches copy pasted from the forum have a tendency to not apply properly.

It should arrive any time now.

First of all, thanks for the patch, I was going to look at this issue just to find that it has been solved in SVN.

One nitpick though. Would it be possible to avoid the generating of the list completely when it is not visible? I.e. setting just some "tc update pending" flag in changeTC, and starting the thread in update() when there is a pending tc change and isVisible() == true?

IMO people usually have the torrent details displayed, not the file list, so this would totally eliminate the useless cpu load.

I just tried to play with the code a little bit more... the version below generates the list only when it is visible, and does not need a separate thread - it uses a timer instead. I didn't test it very thoroughly yet though.

Applies to r681804.

Code: Select all

diff -ub orig/fileview.cpp new/fileview.cpp
--- orig/fileview.cpp   2007-08-27 18:45:42.000000000 +0200
+++ new/fileview.cpp   2007-11-05 18:35:37.000000000 +0100
@@ -41,7 +41,7 @@
 {
 
    FileView::FileView(QWidget *parent, const char *name)
-         : KListView(parent, name),curr_tc(0),multi_root(0)
+       : KListView(parent, name),curr_tc(0),multi_root(0),pending_fill(0)
    {
       setFrameShape(QFrame::NoFrame);
       addColumn( i18n( "File" ) );
@@ -76,6 +76,8 @@
       connect(this,SIGNAL(doubleClicked( QListViewItem*, const QPoint&, int )),
             this,SLOT(onDoubleClicked(QListViewItem*, const QPoint&, int)));
       
+      connect(&fillTimer, SIGNAL(timeout()), this, SLOT( fillTreePartial() ) );
+
       setEnabled(false);
       
       setSelectionMode(QListView::Extended);
@@ -85,30 +87,48 @@
    FileView::~FileView()
    {}
    
-   void FileView::fillFileTree()
+   #define ITEMS_PER_TICK 100
+   void FileView::fillTreePartial()
    {
-      multi_root = 0;
-      clear();
    
-      if (!curr_tc)
-         return;
-   
-      if (curr_tc->getStats().multi_file_torrent)
-      {
-         IWFileTreeDirItem* root = new IWFileTreeDirItem(
-               this,curr_tc->getStats().torrent_name);
+      Uint32 last_item = first_item + ITEMS_PER_TICK;
+      if (last_item > curr_tc->getNumFiles())
+         last_item = curr_tc->getNumFiles();
          
-         for (Uint32 i = 0;i < curr_tc->getNumFiles();i++)
+      for (Uint32 i = first_item;i < last_item;i++)
          {
             TorrentFileInterface & file = curr_tc->getTorrentFile(i);
-            root->insert(file.getPath(),file);
+         multi_root->insert(file.getPath(),file);
          }
-         root->setOpen(true);
+      if (last_item == curr_tc->getNumFiles()) {
+            
+         multi_root->setOpen(true);
          setRootIsDecorated(true);
-         multi_root = root;
+         setEnabled(true);
          multi_root->updatePriorityInformation(curr_tc);
          multi_root->updatePercentageInformation();
          multi_root->updatePreviewInformation(curr_tc);
+         fillTimer.stop();
+         connect(curr_tc,SIGNAL(missingFilesMarkedDND( kt::TorrentInterface* )),
+            this,SLOT(refreshFileTree( kt::TorrentInterface* )));
+      }
+      first_item += ITEMS_PER_TICK;
+   }
+   
+   void FileView::fillFileTree()
+   {
+      multi_root = 0;
+      setEnabled(false);
+      clear();
+   
+      if (!curr_tc)
+         return;
+   
+      if (curr_tc->getStats().multi_file_torrent)
+      {
+         multi_root = new IWFileTreeDirItem(this,curr_tc->getStats().torrent_name);
+         first_item = 0;
+         fillTimer.start(0, FALSE);
       }
       else
       {
@@ -120,6 +140,9 @@
                BytesToString(s.total_bytes));
    
          item->setPixmap(0,KMimeType::findByPath(s.torrent_name)->pixmap(KIcon::Small));
+         setEnabled(true);
+         connect(curr_tc,SIGNAL(missingFilesMarkedDND( kt::TorrentInterface* )),
+            this,SLOT(refreshFileTree( kt::TorrentInterface* )));
       }
    }
 
@@ -129,15 +152,17 @@
          return;
    
       curr_tc = tc;
-      fillFileTree();
-      setEnabled(tc != 0);
-      if (tc)
-         connect(tc,SIGNAL(missingFilesMarkedDND( kt::TorrentInterface* )),
-               this,SLOT(refreshFileTree( kt::TorrentInterface* )));
+      pending_fill = true;
+      fillTimer.stop();
    }
    
    void FileView::update()
    {
+      if (pending_fill) {
+          fillFileTree();
+          pending_fill = false;
+      }
+      
       if (!curr_tc)
          return;
       
diff -ub orig/fileview.h new/fileview.h
--- orig/fileview.h   2007-08-27 18:45:42.000000000 +0200
+++ new/fileview.h   2007-11-05 18:14:20.000000000 +0100
@@ -22,6 +22,7 @@
 
 #include <klistview.h>
 #include <util/constants.h>
+#include <qtimer.h>
 
 namespace kt
 {
@@ -45,6 +46,7 @@
       void showContextMenu(KListView* ,QListViewItem* item,const QPoint & p);
       void refreshFileTree(kt::TorrentInterface* tc);
       void onDoubleClicked(QListViewItem* item,const QPoint & ,int );
+      void fillTreePartial();
       
    private:
       void fillFileTree();
@@ -55,14 +57,18 @@
    private:
       kt::TorrentInterface* curr_tc;
       IWFileTreeDirItem* multi_root;
+      bool pending_fill;
       KPopupMenu* context_menu;
       QString preview_path;
+      QTimer fillTimer;
       int preview_id;
       int first_id;
       int normal_id;
       int last_id;
       int dnd_keep_id;
       int dnd_throw_away_id;
+
+      int first_item;
    };
 
 }


Can you send that by mail as an attachment ? Patches copy pasted from the forum, tend to apply not well.

Whatever you did with this in SVN (738109 I think) - it doesn't work right: the GUI remains unresponsive to interaction and paint events (only the fileview repaints itself) until it finishes loading those files.

J wrote:Whatever you did with this in SVN (738109 I think) - it doesn't work right: the GUI remains unresponsive to interaction and paint events (only the fileview repaints itself) until it finishes loading those files.

Neither previous version (in 2.2.3), nor this one (2.2.4) eliminated blockage of gui for me (I must admit situation got improved a bit though) when selecting a 7000+ file torrent.

MoDaX wrote:

J wrote:Whatever you did with this in SVN (738109 I think) - it doesn't work right: the GUI remains unresponsive to interaction and paint events (only the fileview repaints itself) until it finishes loading those files.

Neither previous version (in 2.2.3), nor this one (2.2.4) eliminated blockage of gui for me (I must admit situation got improved a bit though) when selecting a 7000+ file torrent.

Was my patch included in any of those?

The threaded solution I presented earlier worked perfectly for me, but the current situation is almost just as disturbing as it used to be.

And as far as I can tell, currently it does generate the list even it is not visible.

J wrote:Was my patch included in any of those?

It was in 2.2.3. It has been removed in 2.2.4.

J wrote:The threaded solution I presented earlier worked perfectly for me, but the current situation is almost just as disturbing as it used to be.

The threaded version crashed on SMP systems (includng mine),
Maybe you could try decreasing the number of items added in one go, see #define ITEMS_PER_TICK 100. It is possible that 100 is too much for slower machines.

J wrote:And as far as I can tell, currently it does generate the list even it is not visible.

Unfortunately, yes, since the patch I posted has been modified by Joris.

jmak wrote:The threaded version crashed on SMP systems (includng mine),

Any idea or debug info on why it crashed?

jmak wrote:Maybe you could try decreasing the number of items added in one go, see #define ITEMS_PER_TICK 100. It is possible that 100 is too much for slower machines.

I don't think an AMD Athlon 64 3200+ (2,2Ghz) is too slow for this.

Personally I still think that using the event loop to do this isn't a good idea as everything else on the event queue will have to wait. This might result in loss of performance in other parts of KTorrent.

J wrote:Any idea or debug info on why it crashed?

See http://ktorrent.org/forum/viewtopic.php?t=2064 and other threads when 2.2.3 was released.

Hmm... lol, that did cause a lot of havoc, sry...

I wonder why it did that though... It ran rock-stable on my uniprocessor unicore machine for at least a week.

PS: When debugging multi-threaded programs with GDB, use "thread apply all bt" to get a backtrace on all threads.