From ff4111044b6ad0e200bb58ac73362e05846e1b2b Mon Sep 17 00:00:00 2001
From: Even Rouault <even.rouault@spatialys.com>
Date: Thu, 1 Aug 2024 17:28:43 +0200
Subject: [PATCH] gdalproxypool.cpp: reduce mutex contention, particularly
 useful for VRT multithreading of remove sources

---
 gcore/gdalproxypool.cpp | 57 ++++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 24 deletions(-)

diff --git a/gcore/gdalproxypool.cpp b/gcore/gdalproxypool.cpp
index 8282d7f98c09..2a745aadad31 100644
--- a/gcore/gdalproxypool.cpp
+++ b/gcore/gdalproxypool.cpp
@@ -80,6 +80,17 @@ struct _GDALProxyPoolCacheEntry
     GDALProxyPoolCacheEntry *next;
 };
 
+// This variable prevents a dataset that is going to be opened in
+// GDALDatasetPool::_RefDataset from increasing refCount if, during its
+// opening, it creates a GDALProxyPoolDataset.
+// We increment it before opening or closing a cached dataset and decrement
+// it afterwards
+// The typical use case is a VRT made of simple sources that are VRT
+// We don't want the "inner" VRT to take a reference on the pool, otherwise
+// there is a high chance that this reference will not be dropped and the pool
+// remain ghost.
+static thread_local int refCountOfDisableRefCount = 0;
+
 class GDALDatasetPool
 {
   private:
@@ -100,19 +111,6 @@ class GDALDatasetPool
     GDALProxyPoolCacheEntry *firstEntry = nullptr;
     GDALProxyPoolCacheEntry *lastEntry = nullptr;
 
-    /* This variable prevents a dataset that is going to be opened in
-     * GDALDatasetPool::_RefDataset */
-    /* from increasing refCount if, during its opening, it creates a
-     * GDALProxyPoolDataset */
-    /* We increment it before opening or closing a cached dataset and decrement
-     * it afterwards */
-    /* The typical use case is a VRT made of simple sources that are VRT */
-    /* We don't want the "inner" VRT to take a reference on the pool, otherwise
-     * there is */
-    /* a high chance that this reference will not be dropped and the pool remain
-     * ghost */
-    int refCountOfDisableRefCount = 0;
-
     /* Caution : to be sure that we don't run out of entries, size must be at */
     /* least greater or equal than the maximum number of threads */
     explicit GDALDatasetPool(int maxSize, int64_t nMaxRAMUsage);
@@ -256,6 +254,9 @@ GDALDatasetPool::_RefDataset(const char *pszFileName, GDALAccess eAccess,
                              CSLConstList papszOpenOptions, int bShared,
                              bool bForceOpen, const char *pszOwner)
 {
+    CPLMutex **pMutex = GDALGetphDLMutex();
+    CPLMutexHolderD(pMutex);
+
     if (bInDestruction)
         return nullptr;
 
@@ -340,7 +341,7 @@ GDALDatasetPool::_RefDataset(const char *pszFileName, GDALAccess eAccess,
     {
         GDALProxyPoolCacheEntry *next = cur->next;
 
-        if (cur->pszFileNameAndOpenOptions &&
+        if (cur->refCount >= 0 && cur->pszFileNameAndOpenOptions &&
             osFilenameAndOO == cur->pszFileNameAndOpenOptions &&
             ((bShared && cur->responsiblePID == responsiblePID &&
               ((cur->pszOwner == nullptr && pszOwner == nullptr) ||
@@ -414,15 +415,24 @@ GDALDatasetPool::_RefDataset(const char *pszFileName, GDALAccess eAccess,
     cur->pszFileNameAndOpenOptions = CPLStrdup(osFilenameAndOO.c_str());
     cur->pszOwner = (pszOwner) ? CPLStrdup(pszOwner) : nullptr;
     cur->responsiblePID = responsiblePID;
-    cur->refCount = 1;
+    cur->refCount = -1;  // to mark loading of dataset in progress
     cur->nRAMUsage = 0;
 
     refCountOfDisableRefCount++;
-    int nFlag = ((eAccess == GA_Update) ? GDAL_OF_UPDATE : GDAL_OF_READONLY) |
-                GDAL_OF_RASTER | GDAL_OF_VERBOSE_ERROR;
+    const int nFlag =
+        ((eAccess == GA_Update) ? GDAL_OF_UPDATE : GDAL_OF_READONLY) |
+        GDAL_OF_RASTER | GDAL_OF_VERBOSE_ERROR;
     CPLConfigOptionSetter oSetter("CPL_ALLOW_VSISTDIN", "NO", true);
-    cur->poDS = GDALDataset::Open(pszFileName, nFlag, nullptr, papszOpenOptions,
+
+    // Release mutex while opening dataset to avoid lock contention.
+    CPLReleaseMutex(*pMutex);
+    auto poDS = GDALDataset::Open(pszFileName, nFlag, nullptr, papszOpenOptions,
                                   nullptr);
+    CPLAcquireMutex(*pMutex, 1000.0);
+
+    cur->poDS = poDS;
+    cur->refCount = 1;
+
     refCountOfDisableRefCount--;
 
     if (cur->poDS)
@@ -545,7 +555,7 @@ void GDALDatasetPool::Ref()
         singleton =
             new GDALDatasetPool(GDALGetMaxDatasetPoolSize(), l_nMaxRAMUsage);
     }
-    if (singleton->refCountOfDisableRefCount == 0)
+    if (refCountOfDisableRefCount == 0)
         singleton->refCount++;
 }
 
@@ -555,7 +565,7 @@ void GDALDatasetPool::PreventDestroy()
     CPLMutexHolderD(GDALGetphDLMutex());
     if (!singleton)
         return;
-    singleton->refCountOfDisableRefCount++;
+    refCountOfDisableRefCount++;
 }
 
 /* keep that in sync with gdaldrivermanager.cpp */
@@ -578,7 +588,7 @@ void GDALDatasetPool::Unref()
         CPLAssert(false);
         return;
     }
-    if (singleton->refCountOfDisableRefCount == 0)
+    if (refCountOfDisableRefCount == 0)
     {
         singleton->refCount--;
         if (singleton->refCount == 0)
@@ -595,8 +605,8 @@ void GDALDatasetPool::ForceDestroy()
     CPLMutexHolderD(GDALGetphDLMutex());
     if (!singleton)
         return;
-    singleton->refCountOfDisableRefCount--;
-    CPLAssert(singleton->refCountOfDisableRefCount == 0);
+    refCountOfDisableRefCount--;
+    CPLAssert(refCountOfDisableRefCount == 0);
     singleton->refCount = 0;
     delete singleton;
     singleton = nullptr;
@@ -619,7 +629,6 @@ GDALDatasetPool::RefDataset(const char *pszFileName, GDALAccess eAccess,
                             char **papszOpenOptions, int bShared,
                             bool bForceOpen, const char *pszOwner)
 {
-    CPLMutexHolderD(GDALGetphDLMutex());
     return singleton->_RefDataset(pszFileName, eAccess, papszOpenOptions,
                                   bShared, bForceOpen, pszOwner);
 }