ENH: qvtkConnect and qvtkDisconnect performance improvement

[lassoan]

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)

[finetjul]

That's something I wanted to do for a while. Thanks for fixing it.
I'll do the code review very soon, there seems to be a few thinks I need to look into details.

[lassoan]

@finetjul: Thanks for the comments, I've fixed all of them.

There is one more thing that may worth considering: ctkVTKConnectionTest1 always fails, as the ctkVTKConnection / vtkCallbacks execution time ratio is well above 1.2 (typically between 3-6).
Has this test ever passed? What was changed that increased this ratio so much? (I couldn't find any info on the dashboard - http://my.cdash.org/index.php?project=CTK if this is really the dashboard? seems that nobody cares to submit anything useful here)
Should we raise the ratio limit to something like 10 (to not mask potential other errors) and enter a bug report to investigate the large ratio value? (although I don't really see any way to speed up the QT slot calling)

[finetjul]

Andras, I always had a ratio < 1.2
Here is the ratio I have with your branch (on a mac laptop ):

162: Test command: /Users/exxos/Work/CTK/CTK-build/CTK-build/bin/CTKVisualizationVTKCoreCppTests "ctkVTKConnectionTest1"
162: Test timeout computed to be: 1500
162: 100 events listened by 1000 objects (ctkVTKConnection):  0.190212 seconds 
162: 100 events listened by 1000 objects (vtkCallbacks):  0.163499 seconds 
162: ctkVTKConnection / vtkCallbacks:  1.16338 
162: 100 events listened by 1000 objects (observed ctkVTKConnection):  0.385688 seconds 
162: 100 events listened by 1000 objects (ctkVTKConnection, 1-1):  0.199834 seconds 
1/1 Test #162: ctkVTKConnectionTest1 ............   Passed    1.15 sec

You might have some odd results if you run it in Debug mode (especially on Windows).

[lassoan]

This is interesting. Event execution seems to be very slow on your computer. Is it a debug-mode build? Without any compiler optimization?

On my Win7 desktop (i7, 3.4GHz) in release mode ctkVTKConnection is 7x faster, vtkCallbacks are 32x faster:
100 events listened by 1000 objects (ctkVTKConnection): 0.0279999 seconds
100 events listened by 1000 objects (vtkCallbacks): 0.00499988 seconds
ctkVTKConnection / vtkCallbacks: 5.60011
100 events listened by 1000 objects (observed ctkVTKConnection): 0.029 seconds
100 events listened by 1000 objects (ctkVTKConnection, 1-1): 0.029 seconds

On my Win7 laptop (i5, 2.5GHz) in release mode it's 5x and 27x faster:
100 events listened by 1000 objects (ctkVTKConnection): 0.0350001 seconds
100 events listened by 1000 objects (vtkCallbacks): 0.00600004 seconds
ctkVTKConnection / vtkCallbacks: 5.83331
100 events listened by 1000 objects (observed ctkVTKConnection): 0.0380001 seconds
100 events listened by 1000 objects (ctkVTKConnection, 1-1): 0.04 seconds

On the desktop in debug mode everything is much slower:
100 events listened by 1000 objects (ctkVTKConnection): 2.04 seconds
100 events listened by 1000 objects (vtkCallbacks): 1.836 seconds
ctkVTKConnection / vtkCallbacks: 1.11111
100 events listened by 1000 objects (observed ctkVTKConnection): 2.062 seconds
100 events listened by 1000 objects (ctkVTKConnection, 1-1): 2.035 seconds

It would be nice to see the results on more computers/OSs, but the http://my.cdash.org/index.php?project=CTK seems to be completely abandoned. Is there a CTK dashboard where people actually submit their build results?

[finetjul]

Here is what I have when building in Release mode (on MacBook Pro Lion 2.4Ghz):

137: Test command: /Users/exxos/Work/CTK/CTK-R64/CTK-build/bin/CTKVisualizationVTKCoreCppTests "ctkVTKConnectionTest1"
137: Test timeout computed to be: 1500
137: 100 events listened by 1000 objects (ctkVTKConnection):  0.0534401 seconds 
137: 100 events listened by 1000 objects (vtkCallbacks):  0.0293889 seconds 
137: ctkVTKConnection / vtkCallbacks:  1.81838 
137: 100 events listened by 1000 objects (observed ctkVTKConnection):  0.085551 seconds 
137: 100 events listened by 1000 objects (ctkVTKConnection, 1-1):  0.0563829 seconds

I agree, the CTK dashboard is a bit abandoned unfortunately :-(
Labels don't help to make it look nicer...
Maybe we could turn on the ratio test in Debug mode only ?

[pieper]

This seems like a nice optimization. Could it be rebased? This week would be a good time to merge.

[lassoan]

This has been merged into CTK already. The only outstanding issue is that the tests are failing as acceptable slowdown factor of ctkVTK events vs raw VTK events is 1.2x, while actually ctkVTK event processing is several times slower then pure VTK events. It seems that the bottleneck is Qt, so we cannot do much about it.

Probably we should do two things:

1. Increase the threshold from 1.2x to something like 10x
2. Set up more CTK dashboard computers (http://my.cdash.org/index.php?project=CTK), with submission from at least a Windows, Linux, and Mac

[pieper]

Changing the success threshold so that the test can pass on all normal platforms sounds like the correct answer. The test should confirm the working state of the code, not set a bar that we cannot yet attain.

[jcfr]

+1

On Wed, May 7, 2014 at 10:49 AM, Steve Pieper notifications@github.comwrote:

Changing the success threshold so that the test can pass on all normal
platforms sounds like the correct answer. The test should confirm the
working state of the code, not set a bar that we cannot yet attain.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/378#issuecomment-42436579
.

+1 919 869 8849

[jcfr]

The associated have already been integrated. See a2ade62