ENH: qvtkConnect and qvtkDisconnect performance improvement

Whenever a new connection is added or deleted, it's searched in the list of already existing connections using ctkVTKObjectEventsObserverPrivate::findConnection(). This findConnection() method is very slow if there are many connections, because the time-consuming ctkVTKConnection::isEqual() method is executed for each existing connection (before this commit, 40% of the time was spent in vtkConnection::isEqual while creating 10000 connections).

To solve this performance issue, added a ConnectionIndex structure, which stores a pointer to each connection object using a hash map. When searching for a fully specified connection, the ConnectionIndex is used to find the connection directly, instead of iterating through all the children.

Result of the optimization (ctkVTKObjectTest1):
Create 10000 connections: 2.343 sec => 1.019 sec (2.3x speed improvement)
Remove 10000 connections: 3.599 sec => 2.031 sec (1.77x speed improvement)

Conflicts:
	Libs/Visualization/VTK/Core/ctkVTKObjectEventsObserver.cpp

Libs/Visualization/VTK/Core/Testing/Cpp/ctkVTKObjectTestHelper.cpp

@@ -86,6 +86,30 @@ bool ctkVTKObjectTest::test()
   
   vtkObject* object = vtkObject::New();
 
+
+  int numberOfConnections=10000;
+
+  qDebug() << "Create "<<numberOfConnections<<" connections...";
+  vtkSmartPointer<vtkTimerLog> timerLog = vtkSmartPointer<vtkTimerLog>::New();
+  timerLog->StartTimer();
+  for (int i = 1; i <= numberOfConnections; ++i)
+    {
+    this->qvtkConnect(object, i, this, SLOT(onVTKObjectModifiedPublic()));
+    }
+  timerLog->StopTimer();
+  double t1 = timerLog->GetElapsedTime();
+  qDebug() << "  elapsed time: " << t1 << "seconds";
+  
+  qDebug() << "Remove "<<numberOfConnections<<" connections...";
+  timerLog->StartTimer();
+  for (int i = numberOfConnections; i>=1 ; --i)
+    {
+    this->qvtkDisconnect(object, i, this, SLOT(onVTKObjectModifiedPublic()));
+    }
+  timerLog->StopTimer();
+  t1 = timerLog->GetElapsedTime();
+  qDebug() << "  elapsed time: " << t1 << "seconds";
+
   connection = this->qvtkConnect(object, vtkCommand::ModifiedEvent, 
                                  this, SLOT(onVTKObjectModifiedPublic()));
   if (connection.isEmpty() || object->GetReferenceCount() != 1)

Libs/Visualization/VTK/Core/ctkVTKObjectEventsObserver.cpp

@@ -24,6 +24,7 @@
 #include <QList>
 #include <QHash>
 #include <QDebug>
+#include <QMultiMap>
 
 // CTK includes
 #include "ctkVTKObjectEventsObserver.h"
@@ -82,7 +83,10 @@ class ctkVTKObjectEventsObserverPrivate
 protected:
   ctkVTKObjectEventsObserver* const q_ptr;
 public:
-  typedef std::multimap<unsigned long, ctkVTKConnection*> ConnectionIndexType; // first: originating vtkObject, second: QT connection object name
+  /// ConnectionIndexType:
+  ///  key = hash (generated from the connection parameters)
+  ///  value = QT connection object name
+  typedef QMultiMap<unsigned long, ctkVTKConnection*> ConnectionIndexType;
   ctkVTKObjectEventsObserverPrivate(ctkVTKObjectEventsObserver& object);
 
   ///
@@ -105,16 +109,24 @@ public:
     return q->findChildren<ctkVTKConnection*>();
   }
 
-  static unsigned long generateConnectionIndexHash(vtkObject* vtk_obj, unsigned long vtk_event,
-    const QObject* qt_obj, const char* qt_slot)
+  /// Generate a number from the connection parameters that is most often
+  /// unique for each connection. The slot parameter is not used, as probably
+  /// the same objects with the same event are not connected to different slot.
+  static unsigned long generateConnectionIndexHash(const vtkObject* vtk_obj, unsigned long vtk_event, const QObject* qt_obj)
   {
-    return (unsigned char*)vtk_obj-(unsigned char*)qt_obj+vtk_event;
+    return reinterpret_cast<const unsigned char*>(vtk_obj)-reinterpret_cast<const unsigned char*>(qt_obj)+vtk_event;
   }
 
   bool StrictTypeCheck;
   bool AllBlocked;
   bool ObserveDeletion;
-  
+
+  /// An associative container to speed up findConnection.
+  /// No need to iterate through all the existing connections and check if it is
+  /// equal with the searched one.
+  /// All the connections are present in the index (to allow quick decision that
+  /// a connection does not exist), but a lazy deletion method is used (items
+  /// not necessarily removed from the index immediately when a connection is deleted).
   mutable ConnectionIndexType ConnectionIndex;
 };
 
@@ -152,33 +164,32 @@ ctkVTKObjectEventsObserverPrivate::findConnection(
   const QObject* qt_obj, const char* qt_slot)const
 {
   // Linear search for connections is prohibitively slow when observing many objects
+  // (because connection->isEqual is slow)
   Q_Q(const ctkVTKObjectEventsObserver);
 
   if(vtk_obj != NULL && qt_slot != NULL &&
      qt_obj != NULL && vtk_event != vtkCommand::NoEvent)
-  {
-    // All information is specified, so we can use the index to find the connection
-    std::pair<ConnectionIndexType::iterator, ConnectionIndexType::iterator> rangeConnectionsForObject;
-    rangeConnectionsForObject = this->ConnectionIndex.equal_range(generateConnectionIndexHash(vtk_obj, vtk_event, qt_obj, qt_slot));
-    for (ConnectionIndexType::iterator connectionForObjectIt = rangeConnectionsForObject.first; 
-      connectionForObjectIt != rangeConnectionsForObject.second;
-      /*upon deletion the increment is done already, so don't increment here*/)
     {
-      ctkVTKConnection* connection=connectionForObjectIt->second;
-      if (!q->children().contains(connection))
+    // All information is specified, so we can use the index to find the connection
+    unsigned long hash=generateConnectionIndexHash(vtk_obj, vtk_event, qt_obj);
+    ConnectionIndexType::iterator connectionIt = this->ConnectionIndex.find(hash);
+    while (connectionIt != this->ConnectionIndex.end() && connectionIt.key() == hash)
       {
+      ctkVTKConnection* connection=connectionIt.value();
+      if (!q->children().contains(connection))
+        {
         // connection has been deleted, so remove it from the index
-        connectionForObjectIt=this->ConnectionIndex.erase(connectionForObjectIt);
+        connectionIt=this->ConnectionIndex.erase(connectionIt);
         continue;
-      }
+        }
       if (connection->isEqual(vtk_obj, vtk_event, qt_obj, qt_slot))
-      {        
+        {
         return connection;
+        }
+      ++connectionIt;
       }
-      ++connectionForObjectIt;
-    }
     return 0;
-  }
+    }
 
   foreach (ctkVTKConnection* connection, this->connections())
     {
@@ -197,26 +208,19 @@ ctkVTKObjectEventsObserverPrivate::findConnections(
   vtkObject* vtk_obj, unsigned long vtk_event,
   const QObject* qt_obj, const char* qt_slot)const
 {
-  Q_Q(const ctkVTKObjectEventsObserver);
-
-  bool all_info = true;
-  if(vtk_obj == NULL || qt_slot == NULL ||
-     qt_obj == NULL || vtk_event == vtkCommand::NoEvent)
-    {
-    all_info = false;
-    }
-
   QList<ctkVTKConnection*> foundConnections;
 
-  if (all_info)
-  {
+  if(vtk_obj != NULL && qt_slot != NULL &&
+     qt_obj != NULL && vtk_event != vtkCommand::NoEvent)
+    {
+    // All information is specified, so we can use the index to find the connection
     ctkVTKConnection* connection=findConnection(vtk_obj, vtk_event, qt_obj, qt_slot);
     if (connection)
-    {
+      {
       foundConnections.append(connection);
-    }
+      }
     return foundConnections;
-  }
+    }
 
   // Loop through all connection
   foreach (ctkVTKConnection* connection, this->connections())
@@ -224,10 +228,6 @@ ctkVTKObjectEventsObserverPrivate::findConnections(
     if (connection->isEqual(vtk_obj, vtk_event, qt_obj, qt_slot))
       {
       foundConnections.append(connection);
-      if (all_info)
-        {
-        break;
-        }
       }
     }
 
@@ -366,10 +366,8 @@ QString ctkVTKObjectEventsObserver::addConnection(vtkObject* vtk_obj, unsigned l
 
   // Instantiate a new connection, set its parameters and add it to the list
   ctkVTKConnection * connection = ctkVTKConnectionFactory::instance()->createConnection(this);
-  QString objName=connection->objectName();
-  d->ConnectionIndex.insert(ctkVTKObjectEventsObserverPrivate::ConnectionIndexType::value_type(
-    ctkVTKObjectEventsObserverPrivate::generateConnectionIndexHash(vtk_obj, vtk_event, qt_obj, qt_slot), connection));
-  
+  d->ConnectionIndex.insert(ctkVTKObjectEventsObserverPrivate::generateConnectionIndexHash(vtk_obj, vtk_event, qt_obj), connection);
+
   connection->observeDeletion(d->ObserveDeletion);
   connection->setup(vtk_obj, vtk_event, qt_obj, qt_slot, priority, connectionType);
 
@@ -452,10 +450,29 @@ int ctkVTKObjectEventsObserver::removeConnection(vtkObject* vtk_obj, unsigned lo
     d->findConnections(vtk_obj, vtk_event, qt_obj, qt_slot);
 
   foreach (ctkVTKConnection* connection, connections)
-    {    
-    // no need to update the index, it'll be updated on-the fly when searching for connections
+    {
     delete connection;
     }
+
+  // Only remove shadow connections (connections in the index without a corresponding actual connection)
+  // from the index if the index size grew too big (shadow elements ratio >50% and minimum 100)
+  if (static_cast<int>(d->ConnectionIndex.size())>100+children().count()*2)
+  {
+    for (ctkVTKObjectEventsObserverPrivate::ConnectionIndexType::iterator connectionIt=d->ConnectionIndex.begin();
+      connectionIt!=d->ConnectionIndex.end();
+      /*upon deletion the increment is done already, so don't increment here*/)
+    {
+      ctkVTKConnection* connection=connectionIt.value();
+      if (!children().contains(connection))
+        {
+        // connection has been deleted, so remove it from the index
+        connectionIt=d->ConnectionIndex.erase(connectionIt);
+        continue;
+        }
+      ++connectionIt;
+    }
+  }
+
   return connections.count();
 }
 
@@ -472,3 +489,4 @@ bool ctkVTKObjectEventsObserver::containsConnection(vtkObject* vtk_obj, unsigned
   Q_D(const ctkVTKObjectEventsObserver);
   return (d->findConnection(vtk_obj, vtk_event, qt_obj, qt_slot) != 0);
 }
+