Parallelize snap caching #31374
Parallelize snap caching #31374
Conversation
There was a problem hiding this comment.
Nice to see the initial work towards better snapping cache land!
From a brief look at the code:
- one can't simply run rebuildIndex() in a worker thread - this is dangerous and may cause crashes. One should not do anything with map layer objects in worker threads (for example, while building snap index, the map layer could get removed in the GUI and the object destroyed). For map rendering in worker threads, this is solved by doing two steps: 1. copying anything needed from layers in main thread, 2. doing the actual work in the worker thread - with safe access to everything. This is also the reason why you were getting crashes with postgres connections - the mutex lockers should not be needed if the rebuildIndex() is adjusted to be safe to be run in worker thread.
- I think these changes break the API. Currently I can create a custom QgsPointLocator/QgsSnappingUtils in my python code and use it for spatial queries. With this PR such code would fail because the implementation would be giving me invalid results while the index is being built. I think this needs to be addressed by a new flag in QgsPointLocator/QgsSnappingUtils which would indicate whether the client code does not mind to temporarily get no results while the async index building is in progress
- it would be good to still have the indication that snapping index is being built - maybe using a temporary message bar entry which would get hidden as soon as the indexing is done?
... or by using the task manager for this? |
That would be an option as well, but how would it work if user has some other tasks running in the background - couldn't this in theory block snap index creation for extended amount of time? |
|
I guess we'll need some sort of scheduler / priority handling if this is an issue. |
|
Well, that'll be an issue regardless of whether task manager is used or not. If all threads are busy, e.g. doing background tasks, then the concurrent snapping job will be delayed anyway... |
|
@wonder-sk I made the modifications so the worker thread won't access any unsafe data I also refactor the code so it use the task manager.
|
There was a problem hiding this comment.
Thanks for the updates - they are looking good.
I am still a bit worried about the possible side effects of these changes. There may be existing code calling e.g. iface.mapCanvas().snappingUtils().locatorForLayer(vl).nearestVertex(...) for some operation, which would be now getting invalid matches (at least for a short amount of time), but I am unable to judge how serious this may be. So I guess let's get this merged, and in case of new issues we would add a new parameter to nearestVertex() and similar methods that would indicate whether the client code tolerates invalid matches in case the locator is having its index built.
while building the index
troopa81
force-pushed
the
parallelize_snap_caching
branch
from
c72a000
to
0e98585
Compare
|
Thanks @wonder-sk, I made the modifications |
|
We are waiting for Travis to turn green and I will merge if no objection ... |
|
Can we have the final ok by @wonder-sk ? He has done an excellent job reviewing this PR, so I'd be happy to have his final ok (hopefully soon) rather than merge-for-the-sake-of-freeze. |
|
+1 to merge when travis is green... |
|
@m-kuhn @wonder-sk hugo wan't sure to get back soon enough to merge before feature freeze. Could you merge now this PR is green? |
|
Thanks ! |
|
@troopa81 This has broken some map tools, like the offset tool, which rely on snapping. Can you please investigate? |
|
Also seems to be causing crashes on exit |
|
@nyalldawson Indeed, sorry about that. I'll try to propose a quick fix |
…caching" This reverts commit 87b1aa9
…ing" This reverts commit 87b1aa9
Description
This PR is the main (and last) PR of the proposed GRANT Snap Cache Improvements
For the record the 2 other PRs : #30947 and #31143. Second one is a prerequisite for this PR so it could work without seg fault and dead locks.
The idea of this PR is to parallelize for each layer the snap cache computing (sequential at the moment) using QtConcurrent API and to make it non blocking. As a consequence it is still possible to use QGIS even if snap cache is currently building. User can for instance start to edit node while the snap cache build is in progress.
Snaping information appears for each layer as soon as they're ready, we don't wait all layers to be ready anymore.
As a consequence, there si no progress bar anymore. Indeed, the total number of snap cache to build can vary as we move the cursor in the map.
Here is 2 videos, showing the difference between actual behavior and this PR (on a worst use case).
Actual (17 seconds to build)
This PR (around 7 seconds to build in total)
Implementation details
Snaping informations are retrieved from QgsSnappingUtils (snapToMap) or directly from QgsPointLocator (in QgsVertexTool for instance), so I choose to parallelize in QgsPointLocator::init method.
If the QgsPointLocator needs to be updated (because the user move the cursor outside the index area of interest for instance) but is already being built, the modifications are discarded. The modifications will be taken into account next time (when cursor will move again) once the built is finished. A running QgsPointLocator cannot be modified (to avoid shared memory to be modified from different thread).
TODO
Once we agree on the general behavior, I will write some unit tests and correct existing ones.
Checklist
Fixes #11111at the bottom of the commit messagescripts/prepare-commit.shscript before each commit