Avoid using thread unsafe proj API - approach 2 #4561
Conversation
|
Very nice - I really like where this is going! I would merge initializeCurrentContext(), sourceProjection(), destProjection() into one method (e.g. threadLocalProjData()) that would return QPair<projPJ, projPJ>. This should:
The transformCoords() would call that method once when it starts and keep the projPJ instances locally instead of doing locking + map lookup several times. (micro optimization alert!) The use of QMap::contains + QMap::value can be replaced by QMap::constFind to avoid one extra lookup. |
That's the BEST kind! ;) |
Avoids unpredictable behavior when transforms are being conducted in background threads, such as map renders. Refs qgis#11441 This commit: 1. Uses thread_local storage for projCtx objects, to ensure that every thread correctly has its own projCtx context. 2. Refactors QgsCoordinateTransformPrivate so that the projPJ source and destination objects are instead stored in a map (by projCtx). This allows transforms to be transparently performed using the correct projPJ objects for the particular thread in which the transform is being conducted. This approach avoids expensive detachment of QgsCoordinateTransformPrivate, and allows a single QgsCoordinateTransformPrivate to be safely utilised by QgsCoordinateTransform objects in different threads.
|
@wonder-sk good optimisations, done. |
| static thread_local QgsProjContextStore mProjContext; | ||
|
|
||
| QReadWriteLock mProjLock; | ||
| QMap < uintptr_t, QPair< projPJ, projPJ > > mProjProjections; |
There was a problem hiding this comment.
I think this member would deserve a tiny bit of commenting.
There was a problem hiding this comment.
Nope. Self explanatory.
|
Good job, both of you! |
(Alternative approach to #4553)
While working on #4515 I noticed some odd behavior were QgsCsExceptions were being unpredictably thrown while rendering multiple map layers in parallel. I believe this is caused by QGIS not using the thread safe proj API.
This PR avoids unpredictable behavior when transforms are being conducted in background threads, such as map renders, and is likely a fix for https://issues.qgis.org/issues/11441
This commit:
Uses thread_local storage for projCtx objects, to ensure that every thread correctly has its own projCtx context.
Refactors QgsCoordinateTransformPrivate so that the projPJ source and destination objects are instead stored in a map (by projCtx). This allows transforms to be transparently performed using the correct projPJ objects for the particular thread in which the transform is being conducted. This approach avoids expensive detachment of QgsCoordinateTransformPrivate, and allows a single QgsCoordinateTransformPrivate to be safely utilised by QgsCoordinateTransform objects in different threads.
The advantage of this approach over #4553 is that the thread safety is transparent to API users - there's no need to prepare a QgsCoordinateTransform in advance before using in a background thread. The disadvantage is that the code is more complex, and there's also a small performance hit due to the use of the context->projPj map (and the mutex required to accompany this). Still, of the two I'd prefer this approach...