fix(featurepool): cache management - fixes #58113

Geometry checker cache does not work properly with memory layers.
refreshCache now handles a list of updated features to be thread-safe.

Also, fixes a locker mode, and correctly remove features from spatial
index.
This commit is contained in:
Jacky Volpes 2024-11-04 17:17:47 +01:00 committed by Loïc Bartoletti
parent da86bc17a6
commit bd49d6cf41
8 changed files with 209 additions and 38 deletions

View File

@ -121,15 +121,20 @@ To be used by implementations of ``addFeature``.
the original layer's thread.
%End
void refreshCache( const QgsFeature &feature );
void refreshCache( QgsFeature feature, const QgsFeature &origFeature );
%Docstring
Changes a feature in the cache and the spatial index.
To be used by implementations of ``updateFeature``.
:param feature: the new feature to put in the cache and index
:param origFeature: the original feature to remove from the index
.. note::
This method can safely be called from a different thread vs the object's creation thread or
the original layer's thread.
.. versionadded:: 3.42
%End
void removeFeature( const QgsFeatureId featureId );

View File

@ -121,15 +121,20 @@ To be used by implementations of ``addFeature``.
the original layer's thread.
%End
void refreshCache( const QgsFeature &feature );
void refreshCache( QgsFeature feature, const QgsFeature &origFeature );
%Docstring
Changes a feature in the cache and the spatial index.
To be used by implementations of ``updateFeature``.
:param feature: the new feature to put in the cache and index
:param origFeature: the original feature to remove from the index
.. note::
This method can safely be called from a different thread vs the object's creation thread or
the original layer's thread.
.. versionadded:: 3.42
%End
void removeFeature( const QgsFeatureId featureId );

View File

@ -49,7 +49,18 @@ bool QgsFeaturePool::getFeature( QgsFeatureId id, QgsFeature &feature )
//
// https://bugreports.qt.io/browse/QTBUG-19794
QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Write );
// If the feature we want is amongst the features that have been updated,
// then get it from the dedicated hash.
// It would not be thread-safe to get it directly from the layer,
// and it could be outdated in the feature source (in case of a memory layer),
// and it could have been cleared from the cache due to a cache overflow.
if ( mUpdatedFeatures.contains( id ) )
{
feature = mUpdatedFeatures[id];
return true;
}
QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Read );
QgsFeature *cachedFeature = mFeatureCache.object( id );
if ( cachedFeature )
{
@ -78,6 +89,7 @@ QgsFeatureIds QgsFeaturePool::getFeatures( const QgsFeatureRequest &request, Qgs
Q_ASSERT( mLayer );
Q_ASSERT( QThread::currentThread() == mLayer->thread() );
mUpdatedFeatures.clear();
mFeatureCache.clear();
mIndex = QgsSpatialIndex();
@ -131,19 +143,21 @@ void QgsFeaturePool::insertFeature( const QgsFeature &feature, bool skipLock )
mIndex.addFeature( indexFeature );
}
void QgsFeaturePool::refreshCache( const QgsFeature &feature )
void QgsFeaturePool::refreshCache( QgsFeature feature, const QgsFeature &origFeature )
{
QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Write );
mFeatureCache.remove( feature.id() );
mIndex.deleteFeature( feature );
locker.unlock();
// insert/refresh the updated features as well
mUpdatedFeatures.insert( feature.id(), feature );
QgsFeature tempFeature;
getFeature( feature.id(), tempFeature );
QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Write );
mFeatureCache.insert( feature.id(), new QgsFeature( feature ) );
mIndex.deleteFeature( origFeature );
mIndex.addFeature( feature );
locker.unlock();
}
void QgsFeaturePool::removeFeature( const QgsFeatureId featureId )
{
mUpdatedFeatures.remove( featureId );
QgsFeature origFeature;
QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Unlocked );
if ( getFeature( featureId, origFeature ) )

View File

@ -171,10 +171,14 @@ class ANALYSIS_EXPORT QgsFeaturePool : public QgsFeatureSink SIP_ABSTRACT
* Changes a feature in the cache and the spatial index.
* To be used by implementations of ``updateFeature``.
*
* \param feature the new feature to put in the cache and index
* \param origFeature the original feature to remove from the index
*
* \note This method can safely be called from a different thread vs the object's creation thread or
* the original layer's thread.
* \since QGIS 3.42
*/
void refreshCache( const QgsFeature &feature );
void refreshCache( QgsFeature feature, const QgsFeature &origFeature );
/**
* Removes a feature from the cache and the spatial index.
@ -219,6 +223,20 @@ class ANALYSIS_EXPORT QgsFeaturePool : public QgsFeatureSink SIP_ABSTRACT
QString mLayerId;
QString mLayerName;
QgsCoordinateReferenceSystem mCrs;
/**
* Hash containing features whose geometry has been updated
* but which are not accessible if we get them
* from the layer (not thread-safe).
*
* The philosophy is that as soon as a feature geometry has
* changed, it will be in this hash, and the cache will not
* be used any more.
* \since QGIS 3.42
*/
QHash<QgsFeatureId, QgsFeature> mUpdatedFeatures;
friend class TestQgsVectorLayerFeaturePool;
};
#endif // QGS_FEATUREPOOL_H

View File

@ -153,7 +153,7 @@ void QgsVectorDataProviderFeaturePool::updateFeature( QgsFeature &feature )
}
} );
refreshCache( feature );
refreshCache( feature, origFeature );
}
void QgsVectorDataProviderFeaturePool::deleteFeature( QgsFeatureId fid )

View File

@ -109,6 +109,9 @@ bool QgsVectorLayerFeaturePool::addFeatures( QgsFeatureList &features, QgsFeatur
void QgsVectorLayerFeaturePool::updateFeature( QgsFeature &feature )
{
QgsFeature origFeature;
getFeature( feature.id(), origFeature );
QgsThreadingUtils::runOnMainThread( [this, &feature]()
{
QgsVectorLayer *lyr = layer();
@ -118,7 +121,7 @@ void QgsVectorLayerFeaturePool::updateFeature( QgsFeature &feature )
}
} );
refreshCache( feature );
refreshCache( feature, origFeature );
}
void QgsVectorLayerFeaturePool::deleteFeature( QgsFeatureId fid )
@ -136,14 +139,12 @@ void QgsVectorLayerFeaturePool::deleteFeature( QgsFeatureId fid )
void QgsVectorLayerFeaturePool::onGeometryChanged( QgsFeatureId fid, const QgsGeometry &geometry )
{
Q_UNUSED( geometry )
if ( isFeatureCached( fid ) )
{
QgsFeature feature;
getFeature( fid, feature );
refreshCache( feature );
}
QgsFeature feature;
QgsFeature origFeature;
getFeature( fid, origFeature );
feature = origFeature;
feature.setGeometry( geometry );
refreshCache( feature, origFeature );
}
void QgsVectorLayerFeaturePool::onFeatureDeleted( QgsFeatureId fid )

View File

@ -63,7 +63,8 @@ class TestQgsGeometryChecks: public QObject
};
double layerToMapUnits( const QgsMapLayer *layer, const QgsCoordinateReferenceSystem &mapCrs ) const;
QgsFeaturePool *createFeaturePool( QgsVectorLayer *layer, bool selectedOnly = false ) const;
QPair<QgsGeometryCheckContext *, QMap<QString, QgsFeaturePool *> > createTestContext( QTemporaryDir &tempDir, QMap<QString, QString> &layers, const QgsCoordinateReferenceSystem &mapCrs = QgsCoordinateReferenceSystem( "EPSG:4326" ), double prec = 8 ) const;
QPair<QgsGeometryCheckContext *, QMap<QString, QgsFeaturePool *> > createTestContext( QTemporaryDir &tempDir, QMap<QString, QString> &layers, const QgsCoordinateReferenceSystem &mapCrs = QgsCoordinateReferenceSystem( "EPSG:4326" ), int prec = 8 ) const;
QPair<QgsGeometryCheckContext *, QMap<QString, QgsFeaturePool *> > createMemoryTestContext( QMap<QString, QString> &layers, const QgsCoordinateReferenceSystem &mapCrs = QgsCoordinateReferenceSystem( "EPSG:4326" ), int prec = 8 ) const;
void cleanupTestContext( QPair<QgsGeometryCheckContext *, QMap<QString, QgsFeaturePool *> > ctx ) const;
void listErrors( const QList<QgsGeometryCheckError *> &checkErrors, const QStringList &messages ) const;
QList<QgsGeometryCheckError *> searchCheckErrors( const QList<QgsGeometryCheckError *> &checkErrors, const QString &layerId, const QgsFeatureId &featureId = -1, const QgsPointXY &pos = QgsPointXY(), const QgsVertexId &vid = QgsVertexId(), const QVariant &value = QVariant(), double tol = 1E-4 ) const;
@ -78,6 +79,7 @@ class TestQgsGeometryChecks: public QObject
private slots:
void testAngleCheck();
void testAngleCheckMemoryLayers();
void testAreaCheck();
void testContainedCheck();
void testDangleCheck();
@ -117,6 +119,85 @@ void TestQgsGeometryChecks::cleanupTestCase()
///////////////////////////////////////////////////////////////////////////////
void TestQgsGeometryChecks::testAngleCheckMemoryLayers()
{
QTemporaryDir dir;
QMap<QString, QString> layers;
layers.insert( "point_layer.shp", "" );
layers.insert( "line_layer.shp", "" );
layers.insert( "polygon_layer.shp", "" );
auto testContext = createMemoryTestContext( layers );
// Test detection
QList<QgsGeometryCheckError *> checkErrors;
QStringList messages;
QVariantMap configuration;
configuration.insert( "minAngle", 15 );
const QgsGeometryAngleCheck check( testContext.first, configuration );
QgsFeedback feedback;
check.collectErrors( testContext.second, checkErrors, messages, &feedback );
listErrors( checkErrors, messages );
QList<QgsGeometryCheckError *> errs1;
QList<QgsGeometryCheckError *> errs2;
QCOMPARE( checkErrors.size(), 8 );
QVERIFY( searchCheckErrors( checkErrors, layers["point_layer.shp"] ).isEmpty() );
QVERIFY( ( errs1 = searchCheckErrors( checkErrors, layers["line_layer.shp"], 1, QgsPointXY( -0.2225, 0.5526 ), QgsVertexId( 0, 0, 3 ), 10.5865 ) ).size() == 1 );
QVERIFY( searchCheckErrors( checkErrors, layers["line_layer.shp"], 1, QgsPointXY( -0.94996, 0.99967 ), QgsVertexId( 1, 0, 1 ), 8.3161 ).size() == 1 );
QVERIFY( searchCheckErrors( checkErrors, layers["line_layer.shp"], 3, QgsPointXY( -0.4547, -0.3059 ), QgsVertexId( 0, 0, 1 ), 5.4165 ).size() == 1 );
QVERIFY( searchCheckErrors( checkErrors, layers["line_layer.shp"], 3, QgsPointXY( -0.7594, -0.1971 ), QgsVertexId( 0, 0, 2 ), 12.5288 ).size() == 1 );
QVERIFY( ( errs2 = searchCheckErrors( checkErrors, layers["polygon_layer.shp"], 1, QgsPointXY( 0.2402, 1.0786 ), QgsVertexId( 0, 0, 1 ), 13.5140 ) ).size() == 1 );
QVERIFY( searchCheckErrors( checkErrors, layers["polygon_layer.shp"], 2, QgsPointXY( 0.6960, 0.5908 ), QgsVertexId( 0, 0, 0 ), 7.0556 ).size() == 1 );
QVERIFY( searchCheckErrors( checkErrors, layers["polygon_layer.shp"], 2, QgsPointXY( 0.98690, 0.55699 ), QgsVertexId( 1, 0, 5 ), 7.7351 ).size() == 1 );
QVERIFY( searchCheckErrors( checkErrors, layers["polygon_layer.shp"], 12, QgsPointXY( -0.3186, 1.6734 ), QgsVertexId( 0, 0, 1 ), 3.5092 ).size() == 1 );
// Test fixes
QgsFeature f;
int n1, n2;
testContext.second[errs1[0]->layerId()]->getFeature( errs1[0]->featureId(), f );
n1 = f.geometry().constGet()->vertexCount( errs1[0]->vidx().part, errs1[0]->vidx().ring );
QVERIFY( fixCheckError( testContext.second, errs1[0],
QgsGeometryAngleCheck::DeleteNode, QgsGeometryCheckError::StatusFixed,
{{errs1[0]->layerId(), errs1[0]->featureId(), QgsGeometryCheck::ChangeNode, QgsGeometryCheck::ChangeRemoved, errs1[0]->vidx()}} ) );
testContext.second[errs1[0]->layerId()]->getFeature( errs1[0]->featureId(), f );
n2 = f.geometry().constGet()->vertexCount( errs1[0]->vidx().part, errs1[0]->vidx().ring );
QCOMPARE( n1, n2 + 1 );
testContext.second[errs2[0]->layerId()]->getFeature( errs2[0]->featureId(), f );
n1 = f.geometry().constGet()->vertexCount( errs2[0]->vidx().part, errs2[0]->vidx().ring );
QVERIFY( fixCheckError( testContext.second, errs2[0],
QgsGeometryAngleCheck::DeleteNode, QgsGeometryCheckError::StatusFixed,
{{errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeNode, QgsGeometryCheck::ChangeRemoved, errs2[0]->vidx()}} ) );
testContext.second[errs2[0]->layerId()]->getFeature( errs2[0]->featureId(), f );
n2 = f.geometry().constGet()->vertexCount( errs2[0]->vidx().part, errs2[0]->vidx().ring );
QCOMPARE( n1, n2 + 1 );
// Test change tracking
QVERIFY( !errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeFeature, QgsGeometryCheck::ChangeRemoved, QgsVertexId()} ) ) );
QVERIFY( !errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeFeature, QgsGeometryCheck::ChangeChanged, QgsVertexId()} ) ) );
QVERIFY( !errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangePart, QgsGeometryCheck::ChangeRemoved, QgsVertexId( errs2[0]->vidx().part )} ) ) );
QVERIFY( !errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangePart, QgsGeometryCheck::ChangeChanged, QgsVertexId( errs2[0]->vidx().part )} ) ) );
QVERIFY( errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangePart, QgsGeometryCheck::ChangeRemoved, QgsVertexId( errs2[0]->vidx().part + 1 )} ) ) );
QVERIFY( errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangePart, QgsGeometryCheck::ChangeChanged, QgsVertexId( errs2[0]->vidx().part + 1 )} ) ) );
QVERIFY( !errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeRing, QgsGeometryCheck::ChangeRemoved, QgsVertexId( errs2[0]->vidx().part, errs2[0]->vidx().ring )} ) ) );
QVERIFY( !errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeRing, QgsGeometryCheck::ChangeChanged, QgsVertexId( errs2[0]->vidx().part, errs2[0]->vidx().ring )} ) ) );
QVERIFY( errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeRing, QgsGeometryCheck::ChangeRemoved, QgsVertexId( errs2[0]->vidx().part, errs2[0]->vidx().ring + 1 )} ) ) );
QVERIFY( errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeRing, QgsGeometryCheck::ChangeChanged, QgsVertexId( errs2[0]->vidx().part, errs2[0]->vidx().ring + 1 )} ) ) );
QVERIFY( !errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeNode, QgsGeometryCheck::ChangeRemoved, errs2[0]->vidx()} ) ) );
QVERIFY( !errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeNode, QgsGeometryCheck::ChangeChanged, errs2[0]->vidx()} ) ) );
QVERIFY( errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeNode, QgsGeometryCheck::ChangeRemoved, QgsVertexId( errs2[0]->vidx().part, errs2[0]->vidx().ring, errs2[0]->vidx().vertex + 1 )} ) ) );
QVERIFY( errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeNode, QgsGeometryCheck::ChangeChanged, QgsVertexId( errs2[0]->vidx().part, errs2[0]->vidx().ring, errs2[0]->vidx().vertex + 1 )} ) ) );
const QgsVertexId oldVidx = errs2[0]->vidx();
QVERIFY( errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeNode, QgsGeometryCheck::ChangeRemoved, QgsVertexId( errs2[0]->vidx().part, errs2[0]->vidx().ring, errs2[0]->vidx().vertex - 1 )} ) ) );
QVERIFY( errs2[0]->vidx().vertex == oldVidx.vertex - 1 );
cleanupTestContext( testContext );
}
void TestQgsGeometryChecks::testAngleCheck()
{
QTemporaryDir dir;
@ -1328,7 +1409,24 @@ QgsFeaturePool *TestQgsGeometryChecks::createFeaturePool( QgsVectorLayer *layer,
return new QgsVectorDataProviderFeaturePool( layer, selectedOnly );
}
QPair<QgsGeometryCheckContext *, QMap<QString, QgsFeaturePool *> > TestQgsGeometryChecks::createTestContext( QTemporaryDir &tempDir, QMap<QString, QString> &layers, const QgsCoordinateReferenceSystem &mapCrs, double prec ) const
QPair<QgsGeometryCheckContext *, QMap<QString, QgsFeaturePool *> > TestQgsGeometryChecks::createMemoryTestContext( QMap<QString, QString> &layers, const QgsCoordinateReferenceSystem &mapCrs, int prec ) const
{
const QDir testDataDir( QDir( TEST_DATA_DIR ).absoluteFilePath( "geometry_checker" ) );
QMap<QString, QgsFeaturePool *> featurePools;
for ( auto it = layers.begin(); it != layers.end(); it++ )
{
const QString layerFile = it.key();
QgsVectorLayer *layer = ( new QgsVectorLayer( testDataDir.absoluteFilePath( layerFile ), layerFile ) )->materialize( QgsFeatureRequest() );
Q_ASSERT( layer && layer->isValid() );
layers[layerFile] = layer->id();
layer->dataProvider()->enterUpdateMode();
featurePools.insert( layer->id(), createFeaturePool( layer ) );
}
return qMakePair( new QgsGeometryCheckContext( prec, mapCrs, QgsProject::instance()->transformContext(), QgsProject::instance() ), featurePools );
}
QPair<QgsGeometryCheckContext *, QMap<QString, QgsFeaturePool *> > TestQgsGeometryChecks::createTestContext( QTemporaryDir &tempDir, QMap<QString, QString> &layers, const QgsCoordinateReferenceSystem &mapCrs, int prec ) const
{
const QDir testDataDir( QDir( TEST_DATA_DIR ).absoluteFilePath( "geometry_checker" ) );
const QDir tmpDir( tempDir.path() );

View File

@ -142,29 +142,59 @@ void TestQgsVectorLayerFeaturePool::changeGeometry()
feat.setGeometry( QgsGeometry::fromWkt( QStringLiteral( "Polygon((100 100, 110 100, 110 110, 100 110, 100 100))" ) ) );
vl->updateFeature( feat );
// Still working on the cached data
// Cached data updated with geometryChanged vector layer signal
const QgsFeatureIds ids3 = pool.getIntersects( QgsRectangle( 0, 0, 10, 10 ) );
QCOMPARE( ids3.size(), 1 );
QCOMPARE( ids3.size(), 0 );
// Repopulate the cache
const QgsFeatureIds ids4 = pool.getFeatures( QgsFeatureRequest().setFilterRect( QgsRectangle( 0, 0, 10, 10 ) ) );
QCOMPARE( ids4.size(), 0 );
// Still working on the cached data
const QgsFeatureIds ids5 = pool.getIntersects( QgsRectangle( 0, 0, 10, 10 ) );
QCOMPARE( ids5.size(), 0 );
// Verify that the geometry is up to date
pool.getFeature( 1, feat );
QCOMPARE( feat.geometry().asWkt(), QStringLiteral( "Polygon ((100 100, 110 100, 110 110, 100 110, 100 100))" ) );
// Update a feature to be inside the AOI
feat.setGeometry( QgsGeometry::fromWkt( QStringLiteral( "Polygon((0 0, 10 0, 10 10, 0 10, 0 0))" ) ) );
vl->updateFeature( feat );
// Still cached
const QgsFeatureIds ids6 = pool.getIntersects( QgsRectangle( 0, 0, 10, 10 ) );
QCOMPARE( ids6.size(), 0 );
// Cached data updated with geometryChanged vector layer signal
const QgsFeatureIds ids4 = pool.getIntersects( QgsRectangle( 0, 0, 10, 10 ) );
QCOMPARE( ids4.size(), 1 );
// One in there again
const QgsFeatureIds ids7 = pool.getFeatures( QgsFeatureRequest().setFilterRect( QgsRectangle( 0, 0, 10, 10 ) ) );
QCOMPARE( ids7.size(), 1 );
// Verify that the geometry is up to date
pool.getFeature( 1, feat );
QCOMPARE( feat.geometry().asWkt(), QStringLiteral( "Polygon ((0 0, 10 0, 10 10, 0 10, 0 0))" ) );
// Add enough features for the cache to be full
for ( int i = 0; i < 1100; i++ ) // max cache size is 1000
{
feat = QgsFeature();
feat.setGeometry( QgsGeometry::fromWkt( "Polygon (( 0 0, 20 0, 20 20, 0 20, 0 0))" ) );
vl->dataProvider()->addFeature( feat );
}
// Get all features: fill-in the cache
const QgsFeatureIds ids5 = pool.getFeatures( QgsFeatureRequest() );
QCOMPARE( ids5.size(), 1102 );
// Verify that the features 1 to 102 have been removed from the cache
for ( QgsFeatureId i : ids5 )
{
if ( i <= 102 )
QVERIFY( !pool.isFeatureCached( i ) );
else
QVERIFY( pool.isFeatureCached( i ) );
}
// Update enough features so that the first are removed from the cache
for ( int i = 100; i < 1103; i++ )
{
pool.getFeature( i, feat );
feat.setGeometry( QgsGeometry::fromWkt( "Polygon (( 0 0, 30 0, 30 30, 0 30, 0 0))" ) );
vl->updateFeature( feat );
}
QVERIFY( !pool.isFeatureCached( 102 ) );
// Verify that when we get a feature that is no more in the cache we have the updated geometry
pool.getFeature( 102, feat );
QCOMPARE( feat.geometry().asWkt(), QStringLiteral( "Polygon ((0 0, 30 0, 30 30, 0 30, 0 0))" ) );
}
std::unique_ptr<QgsVectorLayer> TestQgsVectorLayerFeaturePool::createPopulatedLayer()