[OGR provider] Improve performance of subLayers(), particularly on Fi…

…leGDB with the proprietary driver (fixes #18342)
qgis · Jun 2, 2018 · 55aa7a86be56ce098a6c0596d4e4bb191c3a52f5 · 55aa7a8
1 parent 79ba0ee
commit 55aa7a86be56ce098a6c0596d4e4bb191c3a52f5
Unified Split

Showing with 41 additions and 24 deletions.

+33 −18 src/providers/ogr/qgsogrprovider.cpp

+8 −6 src/providers/ogr/qgsogrprovider.h
diff --git a/src/providers/ogr/qgsogrprovider.cpp b/src/providers/ogr/qgsogrprovider.cpp
@@ -198,11 +198,11 @@ void QgsOgrProvider::repack()
       QString errCause;
       if ( mLayerName.isNull() )
       {
-        mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, true, QStringList(), mLayerIndex, errCause );
+        mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, true, QStringList(), mLayerIndex, errCause, true );
       }
       else
       {
-        mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, true, QStringList(), mLayerName, errCause );
+        mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, true, QStringList(), mLayerName, errCause, true );
       }
 
       if ( !mOgrOrigLayer )
@@ -837,18 +837,30 @@ QStringList QgsOgrProvider::subLayers() const
   }
   else
   {
+    // In case there is no free opened dataset in the cache, keep the first
+    // layer alive while we iterate over the other layers, so that we can
+    // reuse the same dataset. Can help in a particular with a FileGDB with
+    // the FileGDB driver
+    QgsOgrLayerUniquePtr firstLayer;
     for ( unsigned int i = 0; i < layerCount() ; i++ )
     {
       QString errCause;
       QgsOgrLayerUniquePtr layer = QgsOgrProviderUtils::getLayer( mOgrOrigLayer->datasetName(),
                                    mOgrOrigLayer->updateMode(),
                                    mOgrOrigLayer->options(),
                                    i,
-                                   errCause );
+                                   errCause,
+                                   // do not check timestamp beyond the first
+                                   // layer
+                                   firstLayer == nullptr );
       if ( !layer )
         continue;
 
       addSubLayerDetailsToSubLayerList( i, layer.get() );
+      if ( firstLayer == nullptr )
+      {
+        firstLayer = std::move( layer );
+      }
     }
   }
   return mSubLayerList;
@@ -4020,11 +4032,11 @@ void QgsOgrProvider::open( OpenMode mode )
     // has precedence over the layerid if both are given.
     if ( !mLayerName.isNull() )
     {
-      mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, true, options, mLayerName, errCause );
+      mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, true, options, mLayerName, errCause, true );
     }
     else
     {
-      mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, true, options, mLayerIndex, errCause );
+      mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, true, options, mLayerIndex, errCause, true );
     }
   }
 
@@ -4045,11 +4057,11 @@ void QgsOgrProvider::open( OpenMode mode )
     // try to open read-only
     if ( !mLayerName.isNull() )
     {
-      mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, false, QStringList(), mLayerName, errCause );
+      mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, false, QStringList(), mLayerName, errCause, true );
     }
     else
     {
-      mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, false, QStringList(), mLayerIndex, errCause );
+      mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, false, QStringList(), mLayerIndex, errCause, true );
     }
   }
 
@@ -4120,11 +4132,11 @@ void QgsOgrProvider::open( OpenMode mode )
     // try to open read-only
     if ( !mLayerName.isNull() )
     {
-      mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, false, QStringList(), mLayerName, errCause );
+      mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, false, QStringList(), mLayerName, errCause, true );
     }
     else
     {
-      mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, false, QStringList(), mLayerIndex, errCause );
+      mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, false, QStringList(), mLayerIndex, errCause, true );
     }
 
     mWriteAccess = false;
@@ -4346,18 +4358,19 @@ QgsOgrLayerUniquePtr QgsOgrProviderUtils::getLayer( const QString &dsName,
           errCause = QObject::tr( "Cannot find layer %1." ).arg( layerIndex );
           return nullptr;
         }
-        return getLayer( dsName, iter.key().updateMode, iter.key().options, layerName, errCause );
+        return getLayer( dsName, iter.key().updateMode, iter.key().options, layerName, errCause, true );
       }
     }
   }
-  return getLayer( dsName, false, QStringList(), layerIndex, errCause );
+  return getLayer( dsName, false, QStringList(), layerIndex, errCause, true );
 }
 
 QgsOgrLayerUniquePtr QgsOgrProviderUtils::getLayer( const QString &dsName,
     bool updateMode,
     const QStringList &options,
     int layerIndex,
-    QString &errCause )
+    QString &errCause,
+    bool checkModificationDateAgainstCache )
 {
   QMutexLocker locker( &sGlobalMutex );
 
@@ -4398,7 +4411,8 @@ QgsOgrLayerUniquePtr QgsOgrProviderUtils::getLayer( const QString &dsName,
         errCause = QObject::tr( "Cannot find layer %1." ).arg( layerIndex );
         return nullptr;
       }
-      return getLayer( dsName, updateMode, options, layerName, errCause );
+      return getLayer( dsName, updateMode, options, layerName, errCause,
+                       checkModificationDateAgainstCache );
     }
   }
 
@@ -4475,7 +4489,7 @@ QgsOgrLayerUniquePtr QgsOgrProviderUtils::getLayer( const QString &dsName,
       }
     }
   }
-  return getLayer( dsName, false, QStringList(), layerName, errCause );
+  return getLayer( dsName, false, QStringList(), layerName, errCause, true );
 }
 
 static QDateTime getLastModified( const QString &dsName )
@@ -4817,7 +4831,8 @@ QgsOgrLayerUniquePtr QgsOgrProviderUtils::getLayer( const QString &dsName,
     bool updateMode,
     const QStringList &options,
     const QString &layerName,
-    QString &errCause )
+    QString &errCause,
+    bool checkModificationDateAgainstCache )
 {
   QMutexLocker locker( &sGlobalMutex );
 
@@ -4835,7 +4850,7 @@ QgsOgrLayerUniquePtr QgsOgrProviderUtils::getLayer( const QString &dsName,
   auto iter = sMapSharedDS.find( ident );
   if ( iter != sMapSharedDS.end() )
   {
-    if ( !canUseOpenedDatasets( dsName ) )
+    if ( checkModificationDateAgainstCache && !canUseOpenedDatasets( dsName ) )
     {
       QgsDebugMsg( QString( "Cannot reuse existing opened dataset(s) on %1 since it has been modified" ).arg( dsName ) );
       invalidateCachedDatasets( dsName );
@@ -5311,11 +5326,11 @@ QgsOgrLayerUniquePtr LoadDataSourceAndLayer( const QString &uri,
 
   if ( !layerName.isEmpty() )
   {
-    return QgsOgrProviderUtils::getLayer( filePath, true, QStringList(), layerName, errCause );
+    return QgsOgrProviderUtils::getLayer( filePath, true, QStringList(), layerName, errCause, true );
   }
   else
   {
-    return QgsOgrProviderUtils::getLayer( filePath, true, QStringList(), layerIndex, errCause );
+    return QgsOgrProviderUtils::getLayer( filePath, true, QStringList(), layerIndex, errCause, true );
   }
 }
 

diff --git a/src/providers/ogr/qgsogrprovider.h b/src/providers/ogr/qgsogrprovider.h
@@ -387,30 +387,32 @@ class QgsOgrProviderUtils
     //! Wrapper for GDALClose()
     static void GDALCloseWrapper( GDALDatasetH mhDS );
 
-    //! Open a layer given by name, potentially reusing an existing GDALDatasetH if it doesn't already use that layer. release() should be called when done with the object
+    //! Open a layer given by name, potentially reusing an existing GDALDatasetH if it doesn't already use that layer.
     static QgsOgrLayerUniquePtr getLayer( const QString &dsName,
                                           const QString &layerName,
                                           QString &errCause );
 
 
-    //! Open a layer given by name, potentially reusing an existing GDALDatasetH if it has been opened with the same (updateMode, options) tuple and doesn't already use that layer. release() should be called when done with the object
+    //! Open a layer given by name, potentially reusing an existing GDALDatasetH if it has been opened with the same (updateMode, options) tuple and doesn't already use that layer.
     static QgsOgrLayerUniquePtr getLayer( const QString &dsName,
                                           bool updateMode,
                                           const QStringList &options,
                                           const QString &layerName,
-                                          QString &errCause );
+                                          QString &errCause,
+                                          bool checkModificationDateAgainstCache );
 
-    //! Open a layer given by index, potentially reusing an existing GDALDatasetH if it doesn't already use that layer. release() should be called when done with the object
+    //! Open a layer given by index, potentially reusing an existing GDALDatasetH if it doesn't already use that layer.
     static QgsOgrLayerUniquePtr getLayer( const QString &dsName,
                                           int layerIndex,
                                           QString &errCause );
 
-    //! Open a layer given by index, potentially reusing an existing GDALDatasetH if it has been opened with the same (updateMode, options) tuple and doesn't already use that layer. release() should be called when done with the object
+    //! Open a layer given by index, potentially reusing an existing GDALDatasetH if it has been opened with the same (updateMode, options) tuple and doesn't already use that layer.
     static QgsOgrLayerUniquePtr getLayer( const QString &dsName,
                                           bool updateMode,
                                           const QStringList &options,
                                           int layerIndex,
-                                          QString &errCause );
+                                          QString &errCause,
+                                          bool checkModificationDateAgainstCache );
 
     //! Returns a QgsOgrLayer* with a SQL result layer
     static QgsOgrLayerUniquePtr getSqlLayer( QgsOgrLayer *baseLayer, OGRLayerH hSqlLayer, const QString &sql );